I want to have a project that is truly open source. So I thought I would try an experiment. I asked defunkt to completely open legacy_woes up to everyone with a github account. So feel free to commit whatever you want, but try to keep it in the scope of the gem/plugin.

I just sent my coworker and email tried out some of the ideas out of my post from two days ago. I sent it to him with a little blurb at the top saying that I'm trying out a new method of code review, and I would love his feedback on it. I would also like some feedback from all of you. Should I word parts differently? Should I scrap the whole idea, and start over? Where can I improve? Do you have any techniques that you find useful?

The files and names have been changed to protect the innocent.

FooController

Lines 235, 236, 237 is there a reason why we are raising exceptions here?
When finding by id why did you choose ActiveRecord::Base#find and not ActiveRecord::Base#first?
Is there a reason for choosing a polymorphic relationship and not Single Table Inheritance(STI)?
How could add_comment(265 to 282) differ if you used STI?
Is it possible to utilize both polymorphic and STI, are there any advantages to doing that?

CommentNotificationTemplate

Line 13 - Any reason why this isn't using some kind of url helper method?
Line 13 - Do we need this line at all if there is a helper method to be used in the template?
Lines 28 - 30: Why is this static method even around?

html.erb
Line 12 - Can we use a helper for this?  Or make a route?

text.erb
Line 8 - Same questions about the url that is assigned in the controller.

In the tests there is a factory being used, but everything the factory includes is overridden.  Is there a reason?

The story template test could really benefit from a url helper.

Francis, this is great work.  I love to see that you got rid of the comment fixture.  I think this is a big step in the direction we need to go.  Your code is getting better all the time.  Keep up the good work.

The follow question was used to provoke thought from the reviewed "is it possible to utilize both polymorphic and STI, are there any advantages to doing that?" I just want him to think with that one. Is this a workable tactic? I would love to help him continue his amazing growth.

I've been in meetings all day so I don't have a lot to write about, but I have to keep up with my blog post a day. So without further ado we need to organize our integration tests.

We've been trying to find out how to better organize our integration testing for a while. We started out by having each test file organized by the controller involved. This works fine for small controllers that aren't interacting with other controllers.

My next idea was to organize them by controller method. That helped slim down a lot of the test files, and put them in a more readable format. This was a great step but when pages have a lot of functionality this too can become a burden, and make test file become lng and unruly.

Tonight I began work on converting one such test file into functional sections. We have a page in the project that has a lot of information, forms, links, et al. I decided that the integration tests for this page should be broken up by functional areas. Anything outside of a functional area will go into a test file for the overall page. The overall test file will contain things like breadcrumb link tests, and tests for user access.

Right now we are using shoulda and webrat for our integration testing. I'm hoping that soon we will have cucumber in the mix.

Doing a code review is difficult for me. I don't always think of the design implications because I'm trying t hurry and get back to coding. I have mainly been checking for tests, and functionality. This needs to change and I think I've got some decent steps to help me make the leap, and use some tools to help illustrate my concerns to my coworkers.

Code Review Cheat Sheet

Great points on how to handle a code review without upsetting the developer of that code.

Flog

A 'golf' score for your code. Anytime code hurts my eyes, flog has a score that backs up my thoughts. Even without the developer knowing how things are being scored numbers seem to work for us.

Flay

Find violations of the dry principle and often fixing these issues can lower your flog score.

Roodi

Points out OO Design principle issues. I haven't used this much, but the few test runs I tried came up with some good suggestions. Really you should just check out the docs for all the things Roodi does.

Reek

Ruby code smell detector. Reek looks for a lot of common code smells and is easily configureable.

I hope that by bringing a new approach to code reviews I can improve our product, team morale, and the skills of everyone involved. I will keep you posted on how my toolbox works out. I'd also welcome any suggestions on source about improving code reviews and/or tools like the above that help focus where I need to spend the most time reviewing.

I'm so tired of running into issues with inheritance. I avoid it like the plague, but unfortunately I have to deal with it in some of the code I work with. Tonight is the night that I lower my tolerance level for inheritance.

I used to think deep inheritance was the only issue. Tonight I was trying to refactor a controller class, but every time I changed something a bunch of other controllers would break. I can handle this for a little while, but this is really starting to get annoying. One level of inheritance still causing pain. The final straw was when I tried to add a before filter to the to controller and all my show methods in the other controllers quit working.

Now I'm off to convince the rest of the team to lower their tolerance levels. We all knows it's bad, but we need to stop drinking a case a day.