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

Conversation

@danscotton
Copy link

I really enjoyed this exercise! Super fun!

I've been reading up a lot on domain modelling and value objects recently, and so my design is heavily influenced by those ideas. I've also been meaning to test out aruba for a while – great little gem!

Design

  • FileParser converts *.sudoko files into a simple array format that Puzzle can use.
  • Puzzle is made up of Row, Column and Box value objects. They all implement the Unit interface (having looked up Sudoku on wikipedia, a Unit is the common term for all of these).
  • I ended up using Matrix in my implementation, and quite liked the interface for Vector when stumbling across it... so I can instantiate objects like Row[1, 2, ..., 9] etc.
  • Puzzle is aware of its own validity and completeness (wasn't too sure about whether this was a Puzzle's responsibility at first, but this is how it ended up. What do you think?)
  • Validator takes a Puzzle and can be sent :status and :errors that can be output back onto the command line. (Unsure as to whether Validator is the right name for this object as it doesn't actually do the validating. Couldn't think of an alternative though...ideas?)

Some things I was mindful of throughout the exercise:

  • Sudoku is not zero-indexed, only programming languages are. I wanted to avoid (n+1) bits and pieces everywhere, so most methods return hashes indexed at 1, instead of arrays. Unnecessary?
  • I didn't want to couple my solution to being a 'only works on the command-line' app. What if we wanted to make this available in a web app, etc.? Same deal with how Puzzle receives its input.
  • I used Numbers instead of Strings for the sudoku numbers. Made sense to me, especially when rules such as all numbers add up to 45 exist in sudoku.

Some things I'm not so sure about

  • The whole value object thing. Think I might've implemented a load of unnecessary :== and :hash gumpf.

Would love to hear your feedback, and I'll be checking out some of the other solutions on here too. These exercises are great – keep them coming!

… inside the spec files. Also left a load of comments in to show my ideas of how I might express the errors.
…red specs into single Unit spec. Added new Box object that also is a Unit.
…dokuValidator stub, as the Puzzle is now aware of its own validity.
…new Validator class. Previous scenarios green, but new steps failing.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if Sudoku::FileParser#parse_rows returned a Sudoku::Puzzle object rather than an array of arrays? That would eliminate this intermediate state. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I like that. At the time I was thinking along the lines of, "What if we needed a Sudoku::JSONParser?" and so thought I needed a common format between the parser and the Puzzle, but you're right – any object that implements the 'Parser' interface/role could always return a Puzzle object.

In terms of testing this with RSpec, would you use expect_any_instance_of(Sudoku::Puzzle).to receive(:new)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@danscotton I agree, all of the parsers could return Puzzle instances.

As far as testing, I would probably do something like expect(parser.parse).to eq my_puzzle. This will work because you re-defined Puzzle#== to check for value rather than identity.

Copy link
Author

Choose a reason for hiding this comment

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

That's true, thanks. If my Puzzle object wasn't a value object, would you go down the any_instance_of route then? I'm sure I've read something from thoughtbot about that being a bit of a smell but I'm not sure. I'll ask it in the forums.

Copy link
Contributor

Choose a reason for hiding this comment

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

It file is not used outside this class, then it should probably be made private.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @JoelQ, will change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

split accepts a regular expression as it's splitting pattern. You could split on non-numeric characters like this:

parse_empties(line).split(/[^d]+/).map(&:to_i)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @JoelQ, didn't know split could do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice syntactic sugar

@danscotton
Copy link
Author

Thanks for all your feedback @JoelQ. Have a good christmas! 🍺

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.

2 participants