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

Katricia - Edges - Calculator #47

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

Conversation

krsmith7
Copy link

@krsmith7 krsmith7 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 variables.
How did you determine what operation to perform? I used case statements with one case for each operator.
Do you feel like you used consistent indentation throughout your code? Yes, though I sometimes am unsure about spacing of code lines that are related but not a part of the same method or block.
If you had more time, what would you have added to or changed about the program? I would make a separate method for getting and checking user input. I would also add modulo, exponents, and the ability to choose how many numbers to use.

@droberts-sea
Copy link

Calculator

What We're Looking For

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

Great job overall! This program works well, and I appreciate the way you've used whitespace and methods to break up your code. There are a few places where things could be cleaned up that I've tried to call out inline below, but in general I am quite pleased with this submission. Keep up the hard work!

print "Second number: "
second_num = gets.chomp
until second_num =~ /\d/
puts number_reprompt

Choose a reason for hiding this comment

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

You've written almost exactly the same code here twice, to get the first number and the second number. Could you DRY that up by putting this logic in a method?

when "divide", "/"
if second_num != 0
total = divide(first_num, second_num)
# If second number is zero, do not invoke method. Assign zero message to total.

Choose a reason for hiding this comment

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

Could you put this division-specific error handling code in the division method?

# Case statement to link input to defined operator method and invoke method to get total
case operator
when "add", "+"
total = add(first_num, second_num)

Choose a reason for hiding this comment

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

This code isn't repeated, but I think it would still increase readability to wrap this case/when section in a method. That would clearly delineate where it starts and ends, and make it explicit what data it needs to work. The method signature might be something like perform_calculation(operator, first_num, second_num).

# Reprompt user until input is one of operators in operator array
until operator_options.include? input
puts "Please input the name or symbol of a valid operator."
input = gets.chomp

Choose a reason for hiding this comment

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

Nice use of .include? here

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