-
Notifications
You must be signed in to change notification settings - Fork 48
Carly Edges - Calculator Project #42
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
base: master
Are you sure you want to change the base?
Conversation
CalculatorWhat We're Looking For
HAHAHAHA CARLY! First of all: I didn't preview the code before I ran it, and I was so surprised that I laughed out loud after I had to wait for the calculation to show up. It was incredibly suspenseful That being said... The code looks pretty good. It works well and the solution is pretty clear. I think largely it's readable and easily understood, not over-complicated. If I were to call out one thing, it would be about your calculation methods: you end up defining your methods inside of your case statement. While this totally works, it's convention to pull out all method definitions into the top most level, so they're not inside any case statements or if statements or loops or what have you. Lastly, you had some questions about organization. If I may make a suggestion...: Your code, as is, is broken up into three parts roughly:
In parts 2 and 3 in your code, you are doing logic based on an operator ... In part 2, you're using an if/elsif/else on checking what operator is equal to, and in part 3, you're using a case statement to check what operator is equal to. Even though it would take some reconsidering of the order of some other parts, if you wanted to re-organize and combine those two checks, I would recommend thinking about that aspect first! Overall, the code is good :) I have a couple of other minor comments, but nothing big. Good work! |
| # Calculations. Certainly not DRY... at least not yet. Can loop through a Method or Hash? Or Method inside a Method or Mathod using a Hash? | ||
| case operator | ||
| when "multiply" | ||
| def multiply (first, second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the code still runs, most style guides, style conventions, and text editors like Atom will complain/give a warning if there's a space between the method name and the parentheses. Aka:
def multiply (first, second)should be this:
def multiply(first, second)| want_explanation = gets.chomp.downcase | ||
| if want_explanation == "yes" || want_explanation == "sure" | ||
| explanation | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just me being nitpicky-- this if ... end block is indented one indent too much!
Calculator
Congratulations! You're submitting your assignment.
Comprehension Questions