WestMichiganRubyTraining / discussion

Create issues on this repository to open discussion threads in the Issue Tracker!
2 stars 0 forks source link

What does Behave correctly mean, also why try-catch instead of testing for values in the code? #15

Open flanderb opened 11 years ago

flanderb commented 11 years ago

This line specifically(and you probably wanted to say -15 not 15) Behave correctly when called with arguments of 15, 0, the string “fifteen” and nil? FizzBuzz as written by everyone seems to work "correctly" for 15, -15, 0, and "fifteen". i.e. FizzBuzz, FizzBuzz , FizzBuzz , "fifteen" respectfully. Seem right to me.

The one issue I had is nil. Bill corrected us when our group decided to put "if nil" as the first test in the if..elsif..elsif part of FizzBuzz. He enthusiastically pointed out that we wanted to CATCH the nil value, implying that we should do a try-catch(begin-rescue in the ruby world). Why a begin-rescue? Why add the complexity if we are just trying to filter out one value(I've never done a begin-rescue, but I've seem 'em and they seem to uglify the code)

billgathen commented 11 years ago

Thanks for pointing out the confusing section. I've amended the post to clarify that those 4 values are arguments to the class itself. It now reads:

Does the class respond correctly when called with arguments of 15, 0, the string “fifteen” and nil (i.e., FizzBuzz.new("fifteen"))?

Responding with "fifteen" would not be correct, but raising an error that says "Please submit a non-negative integer" helps the developer using your class correct the problem. Raising an error is a flag to your caller that says "something bizarre has happened: I can't give you a normal response". This allows the caller to add their own rescue clause to handle these "edge cases" without cluttering up their main program code with analysis of your return value. The caller can decide what to do based on the type and content of the error you raise.

15 is the "happy path" for this logic: the one that proves the class works correctly under ideal conditions. It's often the first test case you write. Not all tests we write are for failures: proving the code behaves correctly is what proves the code does what we intend it to do. -15 is an interesting case, though.

I left "behave correctly" intentionally-vague so that you'd have to make design decisions that you were willing to stand by: should the program fail, should it return an empty value? That's the sort of call you need to make writing production code because you'll never have all the details mapped out in advance. Think in terms of the user of your code: what would be most useful to them?

My opinion is that invalid arguments should cause the code to raise an error (Ruby even helpfully supplies an ArgumentError exception class for this purpose) with a useful error message, which means a message that helps the reader correct the problem (as in my example above), not just say what's wrong.

Can you include working examples for both approaches, so we can see the difference in the code? Details are important for making judgement calls like this.

flanderb commented 11 years ago

This is awesome. As a newbie I've never worked with exceptions before. I've done some reading and I've just realized that the I can throw an exception without having to do a begin, catch etc. I'll work with this tonight.

flanderb commented 11 years ago

So here is my new fizz_single with the exceptions in it. Seems to work. This is a first go at it, so I'm sure there are better ways to do this. https://gist.github.com/flanderb/5447748

billgathen commented 11 years ago

That's excellent work! You've definitely got the idea.

However, since your methods should be the only ones calling the fizz_single method, we know we won't get bogus input like that because we control the input. The question we're trying to answer is "what do I do if the user gives me garbage input like 'fifteen'?". If you're using something like this:

class FizzBuzz
  def initialize last_num
    @last_num = last_num
  end

  # other code using here
end

...then you'll want your tests to be against FizzBuzz.new("fifteen").

I updated the blog post to try and make that clearer. Let me know if it needs more tweaking.

flanderb commented 11 years ago

"since your methods should be the only ones calling the fizz_single"

O.K. help me out here. Why wouldn't we want to expose fizz_single to the outside world. My fizzbuzz does exactly this. I set fizz_single as a class method. I found that this allowed me to more easily test the core functionality of fizzbuzz. Was I wrong to do this?

billgathen commented 11 years ago

That is a very deep question, requiring multiple answers!

Public Interface

When building tools, we try to keep the "public" interface (i.e., the parts the client code can access) as small as possible. This makes it simpler for our users to use our tool properly. We generally do this by (mentally) separating methods for "what we want to do" from methods for "how we do it".

A good analogy is using a key for the ignition of your car. What starts the car? Turning the key in the ignition. How does the car start? The key cylinder closes the electrical connection between the battery and the starter solenoid, which kicks the starter to life, which spins the engine, which starts combusting fuel. No one would buy a car if you had to do all the "how" stuff manually!

We should build software the same way. There are 4 things a user can do in order to take full advantage of our fizzbuzz tool:

  1. Request an object that can calculate fizzbuzz up to a certain limit
  2. Request results as text
  3. Request results as html
  4. Request results as json

This constitutes the "service" our class provides: "what" it can do. How we accomplish those tasks -- calling fb_single repeatedly from inside the generate method, or even having a generate method at all -- should be tucked away so the user doesn't have to worry about them, just like the starter solenoid is hidden away under the hood.

Sometimes we separate what/how methods explicitly by making the "how" methods private, but this is less-common in Ruby than it is in say, Java, and one of the reasons is so our test methods can get access to these "inner" methods, as you point out. I wish it was more common, because it's wonderfully clarifying on this front.

As an aside, you can actually test private instance methods using the .send method, which some testers frown upon, but that's an argument for another day. To test a private fb_single method, you'd say:

FizzBuzz.new(30).send(:fb_single, 3).should eq('fizz')

...with the name of the method as a symbol and any args you want to send to it as arguments to send. If you build a reusable fizzbuzz object in your before call at the beginning of the test suite, you can shrink that down to

fb.send(:fb_single, 3).should eq('fizz')

(Assuming your reusable object was called fb)

Encapsulation

The "should it be a class method?" question is related, but requires a different angle.

The point of using objects is to combine state (data, or the values of our variables) with behavior that depends on that state (our methods). We do this by accepting a limit in our constructor -- fb = FizzBuzz.new(30) -- that we store in an instance variable and use in our methods: our generate method will use the @limit instance variable instead of accepting the value as an argument.

Class methods by definition exist "outside" the object and can't see our internal state or our instance methods. In the past I sometimes made any method that didn't share state a class method, because it didn't "need" to be an instance method. But I now think that's not a valid way to divide things up, because they also become part of the public interface by definition, which means we can't hide them from the user.

In the case of fb_single, it needs the specific value to convert passed in, so it can't share state, which I agree makes it seem like a good candidate for a class method, but it is still definitely a "how" method and therefore shouldn't be exposed to the user so explicitly. By making it a class method, you are implying to the user that it is a method they could be using directly, which is misleading.

To learn more

I wrote a blog series called Object-Oriented Sandwiches as an "Intro to Object-Oriented Design" in a fun, non-language-specific way. It digs deeply into these concepts without getting bogged-down in language details. It might be helpful to get thinking more about these trade-offs by couching them in real-world concepts.