6
\$\begingroup\$

I am new to programming and I would like some feedback. This is my first ruby program that simply adds a + b. I know it's probably more complicated than it needs to be, but I wanted to implement ideas that I was learning. Is there anything that I should do that I didn't or did that I shouldn't?

    puts "<<<-----ADDITION----->>>"
    puts "<<<------A+B=C------->>>"
    letters = %i(a b)
    sum = 0

    letters.each do |let|
      define_method(let) do
        begin
          puts "Enter #{let}"
          let = Float(gets.chomp)
          sum = sum + let.to_f
        rescue
      puts "Something went wrong. Try again."
      retry
    end
  end
end

letters.each do |let|
  send(let)
end

puts sum
\$\endgroup\$
1
  • 5
    \$\begingroup\$ I'm torn; on the one hand, this is waaaay more complicated than it has any reason to be; by that metric, it's pretty 'terrible' code. On the other hand, given the goal of just using a bunch of crazy techniques for fun, the implementation is actually pretty cool. I don't have time at the moment, but I'll probably come back and leave a review later when I get a chance. \$\endgroup\$
    – philomory
    Commented Aug 10, 2016 at 4:01

3 Answers 3

5
\$\begingroup\$

As philomory said in a comment, this is a neat demonstration of some of Ruby's features, but also a horribly complex way of saying a + b.

In it's most basic formulation, you could just do:

puts gets.chomp.to_f + gets.chomp.to_f

Of course, this leaves out a lot of the stuff your code is currently doing (like telling the user what they're supposed to input). So let's see if we can get all the same functionality, but with a different structure.

First, though, a few notes on your current code (apart from the metaprogramming):

  • Don't overwrite arguments just to avoid a new variable. You pass let (a symbol) to define_method, but then use it to store the user's chosen number. So now you have a variable named let, presumably short for "letter", which has nothing to do with letters - now it's a floating point number. You have the contents (the input), but you've overwritten its label. Or, you have the answer, but you've forgotten the question, to paraphrase Hitchhiker's Guide.

    Besides conflating the concepts of name and value, it's also impractical. You wouldn't, for instance, be able to print something like "You entered a = 23", because you no longer know both the letter and the entered value.

  • This is redundant

    let = Float(gets.chomp)
    sum += let.to_f
    

    You create a Float from the input. And then you call to_f on that float - but it's already a float. Either use Float(...) to get a number, or use the .to_f on a string. The latter is the conventional approach, but the former will complain if the input isn't numeric, so that's probably the one you'll want to use here:

    sum += Float(gets.chomp)
    

    And we've avoided overwriting let too.

  • But speaking of code complaining: You might want to be more specific in your exception handling. Your current rescue block doesn't distinguish between the sorts of errors that might arise. It's one thing to complain to the user if their input is bad, but what if the error is about something else? For instance, if I run your code in my editor, it'll run without an interactive terminal. So gets immediately returns nil, which causes a NoMethodError since chomp isn't a method on nil. And since gets returns immediately, the script goes into a retry-loop, spewing "Something went wrong. Try again." as fast as it can until I kill it.

    Obviously, this isn't how the script is meant to be run, but it goes to show how telling the user to "try again" isn't always a solution. Your code just assumes it's always the user's fault, and if it tries enough times, the problem will get fixed. But the error could also be a mistake in the code, like writing get instead of gets - that'd also cause the script to loop indefinitely, even though there's nothing the user can do to salvage things.

Now, for alternative approaches: While define_method is a really neat Ruby feature, it's not called for in this case. But a generalized method for getting a float as user input would be useful. Then you can use that for the rest of the code. For instance, the script could be:

def read_float(prompt)
  puts prompt
  Float(gets.chomp)
rescue ArgumentError # only rescue this type of error
  puts "Please enter a number"
  retry
end

puts %w(a b).reduce(0) { |sum, var| sum + read_float("Enter #{var}") }

Yes, I'm being a little clever with the reduce and everything. A more straight-forward and instantly readable way would be to say:

a = read_float("Enter a")
b = read_float("Enter b")
puts a + b

which is about as to-the-point of "A + B = C" as you can get.

\$\endgroup\$
4
\$\begingroup\$

You may already know this, but just to be sure, since I didn't see this answer yet.

def add(a,b)
  a + b
end
add(4,5) => 9

The simplest way I can think of to define add (I realize you were probably just trying out things!)

\$\endgroup\$
3
\$\begingroup\$

Your indentation is off. The first line should have no indentation, and the begin should line up with the end etc. That code looks more like this:

puts "<<<-----ADDITION----->>>"
puts "<<<------A+B=C------->>>"
letters = %i(a b)
sum = 0
letters.each do |let|
  define_method(let) do
    begin
      puts "Enter #{let}"
      let = Float(gets.chomp)
      sum = sum + let.to_f
    rescue
      puts "Something went wrong. Try again."
      retry
    end
  end
end

letters.each do |let|
  send(let)
end

puts sum

Also that Float and to_f is redundant.


Next, you have a variable issue. let is a Symbol that is used for defining a variable, until this line:

let = Float(gets.chomp)

This reassigns let, and reuses it as a Float, which would be fine, except for the fact that the variable was defied outside the method. this means that it will keep its value between runs. So if you run the a method a second time, it would output something like:

Enter 3.0

Which is not what you want. what you want it to do is reassign the value of the a in a+b=c. The code would be something like this (code modification is marked with comments):

letters.each do |let|
  val = 0 #define outside the method to make it persist.
  define_method(let) do
    begin
      old_val = val #store the old value later
      puts "Enter #{let}"
      val = Float(gets.chomp)
      sum = sum + val - old_val #subtract the old value away, to replace it
    rescue
      puts "Something went wrong. Try again."
      retry
    end
  end
end

With this code you can run the a or b methods again and it will modify sum properly.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.