Skip to content

Instantly share code, notes, and snippets.

@falonofthetower
Last active August 30, 2016 14:20
Show Gist options
  • Save falonofthetower/2fd545f57041482f4177 to your computer and use it in GitHub Desktop.
Save falonofthetower/2fd545f57041482f4177 to your computer and use it in GitHub Desktop.
First off this game is huge. I'm impressed. Most of your code looks to be pretty solid, and your tests are clearly advanced for this stage (we cover testing more extensively later on in the course).
I'm going to concentrate on the area that I see the biggest opportunity for improvement. That is going to be in your modules. First thing I noticed was that you are using `@@global` variables. These are generally frowned on because they can have unexpected side effects. More than that though your structure ends up repeating itself a lot. Many of the methods end up with just one line of difference:
```ruby
break if new_location[0] < Neighborhood.top_left_limit[0]
break if new_location[0] > Neighborhood.bottom_right_limit[0]
```
Which would be fine for a method, but they are surrounded by usually around nine lines of duplicate code. If you need to adjust these methods you have to go to each one of them and adjust all that code for all of them.
My suggestion would be to go back and adjust the structure of these modules. Build them in such a way that you can implement these methods once and adjust their outputs based on the messages you send them. One example would be that you shouldn't have a `RightDiag` and a `LeftDiag` You should have a `Diagonal` that can assume either the type `:right` or `:left` (or possibly inherit from `Diagonal`). I'm even going to suggest you have much in common between `Horizontal`/`Vertical` and `Diagonal` and should possibly combine much of that logic. Another way to put this is that it would appear to me that you are making the wrong distinctions between these pieces.
You have a tendency to use `variable[0]` a lot. I would call this a code smell--especially in ruby.
```ruby
collection.select { |marker| marker.owner.equal? owner }[0]
# ruby makes this easy
collection.detect { |marker| marker.owner.equal? owner }
```
But the other way your are using it is with `location[0]` and `location[1]`. Aren't these x/y coordinates? If so you should label them that way. If you find yourself using `variable[0]` a lot it should give you cause to investigate if ruby has a better way to write it, or to see if there is a better way to label it.
Overall just the fact that you can load all this in your head is impressive. The only thing I don't understand is why you are posting both the test branch and the master branch. The only reason I can think of to have this secondary branch on github would be as a backup or because you are moving between machines. Generally you could probably delete this kind of branch once you are done with it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment