AlexPatriquin/BitcoinMessenger — pull request #1 by JohnKellyFerguson (2014-04-12 17:03:30+00:00)
Title: Initial Code Review: Fixing seed file & Assorted Thoughts

Hey @AlexPatriquin and @chriskohlbrenner, I'm John, a Flatiron School alum and am here to review some of your code.

I just started looking it over. I cloned the repo, fired up the server, started to go through the registration process and noticed the following issue:

After submitting the registration form, I was taken to an error page because the user alex could not be found. It was due to this line of code in the `RegistrationsController`:
```ruby
alex = User.where(:email => "alexpatriquin@gmail.com")[0]
```
I quickly realized that Alex was most likely being created in the seed file, so I ran `rake db:seed` and got some errors. :frowning: They were arising because of the `reply_to_conversation` method. I couldn't find this method defined anywhere in your source code, so I had to go through the Gemfile and try to figure out where it was coming from. `mailboxer` seemed to be the most likely culprit so I went through their documentation. It took a bit of poking around but it turns out that the return value of `#send_message` is a `Receipt`, not a `Conversation`. I updated the code to reflect this and changed the `reply_to_conversation` method to `reply_to_sender`.

### Some Assorted Thoughts

* When working with gems such as `mailboxer` or `devise`, etc., be aware that they each abstract a lot of cool things away that might be worthwhile to try and figure out on your own. In addition, they often occlude what is going on and as a consequence, can make it more difficult to debug issues when they arise. This was the case for me when I was trying to figure out why `reply_to_conversation` wasn't working. Lastly, as your application grows or evolves, the provided functionality of the gem may no longer suit your needs. In these situations, making changes can be more difficult as the gem will typically be more inflexible than the code you write yourself. Just some stuff to keep in mind.

* The `RegistrationsController` is coupled to the seed file. In order for registration to work properly, the db has to be seeded first. The issue with this is that the seed file contains a lot of junk data that is not mission critical. Only the user Alex is needed and yet, in order to get him, we have to create a whole lot of unnecessary data. To improve upon this, I think we have the following options:
* Split the `db:seed` into two separate tasks, something like `db:seed` and `db:load_sample_data`. In `db:seed`, we would create the user Alex. `db:load_sample_data` would create all of the other data in the current seed file.
* In the `RegistrationsController`, replace `alex = User.where(:email => "alexpatriquin@gmail.com")[0]` with a `find_or_create_by`.

* In general, when using numbers in variable names, it's preferred to write the numbers out. Something like `receipt1` would be better as `receipt_one`.

* There's a lot of value in discussion, so I didn't make any of the above possible changes. I think it's much more worthwhile for me to raise an issue and then we can dialogue about it, as opposed to me just making the changes.

* That said, if you like, you should add me as a collaborator to this project. It'll make it easier for me to make small changes and open PRs like this where we can discuss things further.

* I suggest you guys open PRs as well, that way I can review and comment on them. Of course, you don't have to wait for me to approve before you merge a PR. The whole point is that it keeps the conversation thread nice and centralized around a given topic or feature and provides a sort of paper trail.

* You guys have a bunch of [merged branches](https://github.com/AlexPatriquin/BitcoinMessenger/branches?merged=1) still floating in this repo. It's best to delete them if you're not actively developing them.

That's it for now. :+1: :heart: :sunglasses: