Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anibel - Calculator - Edges #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anibelamerica
Copy link

@anibelamerica anibelamerica commented Aug 8, 2018

Calculator

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
Describe how you stored user input in your program? I stored user input in my program using variables and arrays.
How did you determine what operation to perform? I determined what operation to perform using a case...when statement matching user input to allowable operations.
Do you feel like you used consistent indentation throughout your code? Yes, I feel like I used consistent indentation for the program.
If you had more time, what would you have added to or changed about the program? If I had more time, I would add code to trim trailing zeroes from the result so that the program would display an integer or float as appropriate.

@tildeee
Copy link
Collaborator

tildeee commented Aug 12, 2018

Calculator

What We're Looking For

Feature Feedback
Takes in two numbers and an operator and performs the mathematical operation. x
Readable code with consistent indentation. x

Hi Anibel! Your calculator project looks great. I think that your methods are really clever-- not only did you separate/organize your code into sections by creating methods, but you thought of the code deeply enough to see opportunities to separate them by responsibility. I want to particularly highlight something really cool you did: You made your print_result method, whose responsibility is to display some given string to the user, and separated it from the actual calculation, whose responsibility is to calculate... and not worry about string formatting or output.

If I were to make a major recommendation for your code, it would be: your calculation methods return an array numbers, whose index 0 is the first number, index 1 is the second, and, if applicable, index 2 is the result. It's a little cumbersome to remember that numbers[2] is some result which may not exist (because of division). Also, since it's an array, in these methods you shoveled the information into numbers[2], which isn't very clear to me.

Maybe numbers could be a hash, with the keys first, second, and result. Also, maybe in the method divide(), there should always be something put into numbers that represents the result, such as nil, "Infinity", or... Ruby actually has a way to express the value of infinity with Float::INFINITY. Then, it can be the job of the method print_result to handle that case and format it :)

I'm adding a few other comments for minor improvements, but they are simply small suggestions.

In general, I think that the code is extremely clean and clever. Great job!

when "modulo", "%"
valid_prompt = true
print_result(modulo, "%")
else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case statements, the "catch all" for "nothing matched" is usually default, not else


# calls the function for printing results depending on operator
# the arguments of print_result are evaluated so that an array and a string are passed
case operator
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for every when in this case statement, there's the repeated line valid_prompt = true.

Instead of repeating yourself, maybe you can invert this-- before the case statement, say valid_prompt = true. Then, if it hits the default (because nothing matched), you can say valid_prompt = false

["first", "second"].each do |order|
puts "Enter the #{order} number:"
num << is_a_num(gets.chomp)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants