Skip to content
This repository was archived by the owner on Jun 8, 2019. It is now read-only.

Conversation

@GCorbel
Copy link

@GCorbel GCorbel commented Oct 16, 2013

I'm not very proud of my #grids method but I don't see another solution. And you?

@JESii
Copy link

JESii commented Oct 17, 2013

@GCorbel - nice work on your Validator. Here are a few comments:

  1. I don't see a list of the errors for an invalid Sudoku?
  2. I like your use of symbols in your check_validity_for() methods; much nicer than the strings that I used (I'll have to change mine).
  3. I like your use of shared_examples in sudoku_validator_spec.rb... makes that part of things clear and avoids repeated code for that test.
  4. However, it seems to me that there is a lot of duplicated code in your validator specs - those reader methods that are defined in many of the describe blocks I find disconcerting: they break the flow of the code and make it more difficult for me to understand. I'd look at this and see what you might be able to do.
  5. I think the sudoku_reader_spec code can be dried up so that you don't have to repeat the 'reader = SudokuReader.new...' code in each example. You could, for example, make two context blocks - one for 'valid_complete' and one for 'valid_incomplete' and then instantiate reader in a before block.
  6. I like your convert_line_to_values method and the associated sanitize method; nice, clean conversion of the input data to a usable format
  7. I'm confused by your fill_blocks method: I don't understand why you have three loops of 3 each rather than 2 loops to fill in the 9 elements in a block. Then you use a range ([k...k+2]) in the innermost loop. It looks like you're overlaying data along the way to wind up with the final block?
  8. Maybe it's just me, but I'm uncomfortable having SudokuValidator.new silently do all the work. To me, it's not exactly clear what's going on. My style would be to first initialize the instance, then call a method such as validate(). That way, I better understand what's going on. Also, additional methods could be added in the future like, maybe, a method to suggest fixes or provide hints? I'll be interested to see what other folks have to say about this.
    Anyway, congratulations!

Copy link

Choose a reason for hiding this comment

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

i find this method hard to read. i would prefer if it just initialized blocks and you filled it somewhere else. something like:
@blocks ||= 9.times { @blocks << [] }

no reason to keep the global @blocks when you can call this helper

@GCorbel
Copy link
Author

GCorbel commented Oct 17, 2013

@JESii
1 - Oops, I didn't read this part;
5 - I will check it;
7 - I agree and I will check it. I'm not very proud of this part;
8 - I don't have any problem with this behaviour;

@GCorbel
Copy link
Author

GCorbel commented Oct 18, 2013

I made some changes. I think it's much better. What you think?

Copy link

Choose a reason for hiding this comment

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

Wow... This is the reason why I love ruby; I can't believe transpose is a standard lib method

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants