class DirtyInformation

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.

I'd really like to get better at writing, and get more of the thoughts out that are always clogging up my brain. My wife is an English teacher and I could have her proof everything, but I don't want to bother her with that. Since practice makes perfect, I'm going to try to write something every day! I'm just warning anyone who might actually read this stuff, you might see a lot of gibberish posts when I'm just trying to force something out.

Stop URL Parameters(SUP)

Any time I see a ?foo=bar in a url I get so frustrated. I'm especially irritated when it the parameters came from a form. Let's stop this horrible practice. If you need parameters great, but pass them with a post or as part of the regular url.

I've seen this happening at an increasing rate. It usually comes with someone trying to follow RESTful principles with their code, but becoming confused along the way. Here is an example:

• I have a few reports that I want to display based on some dates
• ...I know...that is a "show" method since I'm "showing" data to the user
• Now I need the report to be based on a date range.
• I also need a select box for the report type to be chosen.
• I'll make a form that posts to this action here, and then it will redirect to the correct report.
• hmm...How am I going to pass those dates along...Eureka! I'll just put ?start_date=foo&end_date=bar on my URL

Now is the time to say STOP! Why do you want to do this. Sit back, and rethink your actions. Starting down the path of just tossing the parameters into the url can lead to many problems. It is like a gateway drug. It seems so easy and useful the first time, and now you've got secure data in the url, and a horrible API, and we won't even talk about error handling and the back-end code that is making this up. Now that your boss is angry that you passed an SSN in the url you would like to go back and fix this issue once and for all.

It is OK to display data with a create or update. If you have a mental block on this think of it as creating the report. Then instead of having a new controller for each report, and a show action for each have the create action render a specific template based on the report selected. The partial can even be the name of the report, then you don't need an if/case block. Now you've got one small method that is handling all the reports. If they are complicated create some models for the reports, and your set.

I've been wanting to get that off my chest.