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

Karis - Edges - Calculator #43

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

Karis - Edges - Calculator #43

wants to merge 1 commit into from

Conversation

kimj42
Copy link

@kimj42 kimj42 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 methods.
How did you determine what operation to perform? I determined what operation(loop, method, etc) to perform by looking at the criteria for the assignment and finding the operation that would make the code concise.
Do you feel like you used consistent indentation throughout your code? Yes, I used consistent indentation throughout my code.
If you had more time, what would you have added to or changed about the program? If I had more time, I would have printed out the formula in addition to the result and add support for computing exponents.

@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

Karis!

Your project works as expected -- it does all the calculations. Good work on that!

There are a few things that strike me as a little strange about your code--

  • In order for you to assign changes to the variable answer both in and outside of methods (like your formatting methods), you made answer a global variable named as $answer. While it works, it would have been better in my opinion to keep $answer as a local variable and to pass it in as a parameter. For example, right now you have this method:
def adding_format()
  ($answer =~ /[+]/ && $answer.length == 1) ...
end

I'd prefer this:

def adding_format( operation_input)
  (operation_input=~ /[+]/ && operation_input.length == 1) ...
end

This allows variables to use local scope instead of global scope.

  • In general, I think that $answer represents the user input that the program gets when asking for an operation, so I would prefer a different variable name than answer (such as operation_input)

  • Your formatting methods essentially ask "is the value in $answer matching the strings with these exact characters," and then you return true or false if it matches one of those formats... which works! I think that if you look at this with fresh eyes, there may be better ways to check "How do I know that this string is one of the strings that I expect it to be."

  • in the calculate method, you eventually get the value of the second number from the user using gets.chomp and put it into the variable sec_num. Then you check if sec_num is zero with sec_num == 0... however, gets.chomp (and therefore, sec_num) gets a string, so it will never equal 0... just "0". Therefore, in your program, I can divide by zero and get an error that breaks your program!

  • In line 48, you end up calculating everything with first_num.to_i.send(change_operator, sec_num.to_i). I would prefer that you don't get in the habit of using the .send method... even though it works in this case, it's a powerful method that would be better to not use much.

I think your approach to solve calculator isn't typical/isn't common -- which isn't a bad thing! But I wonder if you made it a little more complex for yourself than needed. When you feel like you're ready to look at this project with fresh eyes, I'd encourage you to ask a fellow classmate if they can walk you through their code, and maybe you can see how you two approached things the same way (you asked for input, verified it was in a format that you needed and was a correct operation, and then made a calculation based on that operator), but did different implementations.

In general, the format of the code is good, and you got it to work, and I'm happy :)

Let me know if you have any questions!

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