sherfin94 / resume-maker Goto Github PK

Have a look at these tests. All these are tests pertaining a single method. The first test is actually not needed and the second redundantly checks the final result. Here's a better way to rewrite the same:

describe "#to_hash" do
  it 'returns a hash with name, age, place' do
    the_hash = user.to_hash

    expect(the_hash).to eq({name: 'Some Name', age: '21', place: 'Some Place'})
  end
end

As a convention, when testing methods, have all the individual tests wrapped in a describe block as above. If the method is an instance method, let the description be the method name preceded by a hash # (e.g. describe "#to_hash"). If it were a class method, let the preceding symbol be a period . (e.g. describe ".my_class_method").

When we write tests, we don't usually check whether an instance of a class responds to a particular method. Instead, we check whether that method behaves correctly given various situations/scenarios. Your tests should ensure that in the event of code changes, the original functionality is still intact (these are what we call regression tests - just a fancy term).

Let's look at the tests written for the plugin handler and the resume maker. I can change the internal implementation of the methods and the tests wouldn't catch the changes.

Let me know if you need help understanding this more.

The parent class for plugins is empty. Which means that it is not really needed there in the first place. I'm not suggesting that you get rid of it. What I'm saying is that in its current form, that class does not serve any specific purpose. Here's a small exercise for you - check out the template pattern. That might help you identify what that class should do.

As a brainstorming exercise, can you try figuring out if there's any way you can avoid using the case block here?

Usually the switch-case block is regarded as unclean code. I don't quite agree with that. What I do think is that the tendency is generally to misuse it and hence it ends up getting used in inappropriate places. I think your current use of it is fine. But like I mentioned, as an exercise, just think about other means through which you can implement the same (no need to update the code or anything).

When writing a test, there are generally 3 parts - arrange, act, assert.

• Arrange refers to the setup necessary for the test happens - setting up objects, assigning values, etc.
• Act is the actual action - the main line that produces the effect the test is checking.
• Assert refers to the 'test' part, where you're checking the output of the action and the desired result.

Keep these sections separated by a space.

Let's have a look at one of the tests you've written. Right now, all the lines are not separated in any way. Here's how to space them apart according to the scheme I've mentioned above:

it 'returns a hash with name, age, place when called to_hash' do
  the_hash = user.to_hash

  expect(the_hash).to be_a(Hash)
  expect(the_hash).to eq({name: 'Some Name', age: '21', place: 'Some Place'})
end

In the resume maker, there are attribute accessors specified for the handlers. But if you really look at it, these are not details that need to be exposed outside this object. One of the ideas of OOP is encapsulation - keep internal stuff internal.

Let's take a look at this spec. It tests a method whose sole responsibility is to invoke another method. But the test does not check that part of it. I can change the method to return 1 at the end no matter what and the tests would still pass. A simple fix here can take care of that. There are a couple of other places where this has happened. I think this example should help you figure the rest out.