SudokuValidator submission

[JESii]

Great challenge! Here's my take on the SudokuValidator. I'm looking forward to feedback. while I continue to work on improving the code.

[GCorbel]

The bin file is missing but you did a very great work!

For your #convert_line_to_row method, you don't need to convert '.' to '0' because '.'.to_i give 0.

I thinks it's better to give an array with rows and columns instead of col and row methods. You must to build rebuild the column each time you ask it. It's not very effective.

You have one test file but the files with code. I thinks it's better to split it. In your executable file, I think you can place a method to build the message in the validator class. You probably should had more spaces in your spec file too.

In your spec file, you do @game.game.board, it does not rspec the law of demeter. There is probably a better think to do.

You have an empty before block in your tests.

You did that @game.game.board[0][0] = 0 ; @game.game.board[0][6] = 0. Why not two lines?

At least, I think you can move the analyze method in the initalize method and make it private.

[JESii]

@GCorbel - thank you very much for your comments; much appreciated. I'll respond here to each of your points and then go dig into the code for a bit.

1. The bin file is there; it's just not in a separate /bin directory, if that's what you mean.
2. #convert_line_to_row and '.'.to_i = 0. True, but I'm a fan of being explicit on conversions, lest I forget the 'default'
3. What I think you're saying is to build the columns in advance so that they don't have to be rebuilt each time. True but in this case, I believe I only access each column one time, so I'm not sure it's worth making that change?
4. What I get from this is that:
   (a) I should consider separating the specs into two files, one for SudokuBoard and one for SudokuValidator... right? I agree.
   (b) add a method to build the message; I'll take a look at that
   (c) blank lines in spec file; agreed
5. Law of Demeter - great point; thanks! I'll look at refactoring that.
6. Empty before block will disappear
7. Two initializations on one line; no particular reason. I started out with only two changes, and then later decided to add a third.
8. Good point; I'll take a look at that. In the past, I haven't worreid too much about public vs private except in Rails controllers.

Thanks again for your feedback!

[elubin]

it's a pet peeve, but i don't think these should be left as comments in the file. git history can be used if necessary.

[JESii]

Agreed, @elubin; I overlooked them and will delete them.

[mrhead]

What about creating own describe block for each method? Something like this:

describe SudokuBoard do
  describe '#row' do
    ...
  end
end

I think that it improves readability of tests and you can group tests for each method together.

[JESii]

Thanks @mrhead... I wasn't sure at first if I thought this was a good idea: I like the idea of a describe block to separate complete/incomplete board examples - that's the way I started my development. However, after some reflection I think your suggested refactoring makes sense... while not immediately necessary, in the long run it will more clearly isolate the individual elements being tested. I'll work on that fairly soon.

[JESii]

OK; done. The only thing I'm not sure I like about this is the before block which now loads the valid_complete file and the invalid_incomplete file to support the different tests, creating @game_vc & @game_ii instances. I don't think that creating additional describe blocks makes sense -- lots more code and repeated before blocks.

[mrhead]

I've stopped using before and let some time ago. Purely because of this: https://github.com/thoughtbot/guides/tree/master/best-practices#testing

From beginning I did not know how to replace it, but now I create my own methods to prepare objects for me. So unlike in before block I call them just when I need them. And unlike the let, I can pass arguments to it... So in your case you can do something like:

describe SudokuBoard do
  ... your tests ...

  def game_vc
    SudokuBoard.new("valid_complete.sudoku")
  end

  def game_ii
    SudokuBoard.new("invalid_incomplete.sudoku")
  end
end

And then you just call `game_vc` instead of `@game_vc` which doesn't looks right to me.

Anyway this is just something what I made up from different codes and probably also thoughtbot TDD workshop (I really do not remember).

And I would also like to see different opinion on this.

[JESii]

I see what you mean. I'd like to see some explanation as to why some if these are considered the way to do things...

Sent from my iPhone

On Oct 22, 2013, at 7:06 AM, Patrik Bóna notifications@github.com wrote:

thoughtbot