Tag Archives: code

The 3 Common Types of Code Reviews

Much of my development involves reviewing and reading code, both formally and informally. You will only write code once, might modify it a few times, but you will read it over and over. I found code reviews are just a formalization of this code reading process.

I’ve noticed a few different types of code reviews, all based on what the goal of the review is.

Getting to Know You Code Review

The Getting to Know You code review is done so you can understand what the code is doing in general. Typically these are done on an entire application or library as a whole, where there is a large volume of code to review.

The goal of this is to get to know the code and be able to navigate around. What I try to do is to get a good understanding of the major components and how they interact. For Ruby on Rails applications this means the relationship between:

  • the urls (routes) and controllers – how the users will see the application’s workflows
  • the controllers and the models – how the data will be retrieved and updated
  • the models to other models – how the data is related

A few things to watch out for are God Objects, too many classes, two few classes (both symptoms of design problems), and a lack of tests.

We Have a Problem Code Review

The We Have a Problem code reviews are a different beast. These come up because of a specific problem in the application: maybe there is a bug, performance has degraded, or a (human) process has gone bad.

This type of code review is very similar to the process you use when trying to reproduce a bug, in fact I’d consider reproducing a bug to be a code review of sorts. Typically these code reviews follow a process like:

  1. Create a hypothesis of the cause of the problem,
  2. attempt to either prove or disprove the hypothesis through experiments,
  3. review the results,
  4. Refine the hypothesis and iterate or discover of the bug’s root cause.

(This is similar to the Scientific Method)

Typically you’d start with a very broad hypothesis and narrow it down as you cycle through the steps. Finally you’d hone in on the problem areas and then have enough to be able to come up with a solution.

When you compare this type of review to the Getting to Know You, the We Have a Problem has a few major differences:

  • the goal of this is to find a problem so it can be fixed
  • the code needs to be understood just enough to solve the problem
  • the review starts very broadly and then narrows down its search quickly

So in this case, the code review is merely a means to an end, fixing the problem.

The Refresher Code Review

The Refresher code review is designed to get back up to speed on how some code works. You might have written the code in the past, or maybe it is some code you’ve used before. In either case, the code is familiar but you need to do a quick review to become reacquainted with it. Typically these reviews are short, very informal, and most developers do them without thinking about them. Whenever you open up a class and scan through its methods and data, you’re doing a Refresher code review.

There isn’t much to say about the Refresher. It’s a lot like the Getting to Know You in that it can be broad, but it’s like the We Have a Problem in that you might only focus on a few classes or objects.

The important thing to remember is that the Refresher’s goal is to help you understand a section of code again so you can work on something.

Many Subtypes

These are a few different types of code reviews that I do and have seen other people do. They encompass many of the different reasons someone will want to do a code review and there are a few subtypes of code reviews that are used in specific cases. Like starting a rescue project (Getting to Know You) or a security audit (We Have a Problem). There are probably other major types of code reviews but these are the ones I come across again and again.

There is one final type of code review that I think developers should do more of (no, not a dramatic reading of the code)…

One Last Type, For Fun

That’s doing a code review for fun, enjoyment, and to learn. Sitting down with a chunk of code, a tasty beverage, and spending an hour reading and reasoning through some code just for the fun of it. I’ve done it with prototype.js, Rails, and capistranio in the past and that knowledge is still helping me today.

[box type="tick" style="rounded" border="full"]If you’ve never done a code review for enjoyment, post what you’re interested in (topic) and what programming languages you know in the comments below and I’ll try to make a recommendation for you.[/box]

They don’t take a lot of time and you can do them with very little equipment (I’ve printed out code before and also read some on my iPad).

What I’m excited about with ChiliProject 3.0.0

On February 6th ChiliProject 3.0.0 was released. This marks the third major release for ChiliProject and the stabilization of the past 6 months of development.

The 3.0 series is my favorite release so far. It finally brings in some features that I’ve been wanting to use for at least 4 years. The full release details is on the official ChiliProject blog but I wanted to outline a few of the key features that I’m going to be using (and how I’m going to use them).

New Design

The most visible change in 3.0 is the new design. This is a cumulation of the work I did with Shane Pearlman and Peter Chester of Modern Tribe (formally Shane and Peter Inc). Originally this design was a theme plugin that was used by half a dozen companies.

Late 2011 the design was moved from the plugin into the core code to become the default theme. After that the developers at Finn Labs created another major redesign and let the ChiliProject developers iterate on it to create the final design that you see now.

The design is still a work in progress (as all designs should be) but there is already a huge improvement over the previous one.

Liquid

Liquid is another major feature that made it into ChiliProject 3.0. It started as custom development for a client of mine, which was then updated and merged into the core by Holger Just.

Liquid replaces the basic wiki macro system in ChiliProject with a powerful templating system (you could call Liquid a programming language). What this means is that every “wiki” text field will accept Liquid and will let the user write and run small programs, without having to touch the server or Ruby.

I posted an early example of Liquid in action on my personal tumblelog. (full size)

I’m excited for Liquid because I use wiki pages a lot for creating simple project dashboards for clients. It’s easier to use a wiki page and create the dashboard by hand then to write some custom code for each client. Now with Liquid, I can automate some of the data collection and make the pages even more dynamic.

Tagging

Tagging support was also added to ChiliProject. It’s not complete since it is only the underlying data structures (i.e. database tables). The good news is that by having the data structures in place, the rest of tagging can now be added in a minor release (like 3.1.0 or 3.2.0).

Personally, I’m looking forward to issue tagging. Like many people the issue categories are too rigid and tags would be easier to manage. I’m hoping I’ll get to add issue tagging myself, I have a prototype of them from a few months ago that I can port to the core code.

jQuery and jQuery UI

On the development side there are two huge features included in ChiliProject 3.0 that will make writing plugins easier.

First, ChiliProject is now including jQuery and jQuery UI in the core. Every one of my 70 or 80 plugins that have a user interface have been using jQuery. This means that each one has to include the actual jQuery library and use hooks to get jQuery included in the main layout. That’s a bunch of code that I’ve had to repeat over and over again.

With jQuery now being supplied by the core, I can remove all of that code and know that the plugin will always have jQuery available.

Having jQuery in the core also means that the core itself will start using it to provide a more interactive experience. A lightbox style popup has already been included and used to show issue description changes.

(Note: There is a method you can use to check if jQuery is included in ChiliProject. This is useful if you are trying to support multiple versions of ChiliProject or Redmine and want to load jQuery only if it’s not included. Use ChiliProject::Compatibility.using_jquery?.)

Capybara

The second feature that will really help plugin development is the inclusion of Capybara. Capybara is a fake browser that is used in the test suite and can simulate a actual person filling out forms and clicking links.

This is going to help make the tests more through and catch problems with activity flows (e.g. logging in and creating an issue, replying to a forum post). My hope is that with a powerful tool like capybara, more interactions will be tested in the core and in plugins, which will lead to better code and less bugs.

I’m really excited about ChiliProject 3.0.0. It lays down a great foundation that can be built on to improve the usability and user interaction.

Open Source Friday #4 – 2011-05-27

Last week’s Open Source Friday was a busy one so I’ll get right into it:

#425 Deprecation warning when using ChiliProject with Rake 0.9: Rake has had a recent update that seems to have deprecated some older APIs. Starting to feel like it’s deprecation season in Rails-land these days.

#202 Adding the theme used on chiliproject.org to the repository: ChiliProject.org has been running on a custom version of the Squeejee theme since we launched. At the request of several people I included the theme into the main repository for the 1.4.0 release. It isn’t the default theme but will be available as an option.

#424 Loading issue context menu causes two identical AJAX requests: Gregor Schmidt and Emilio Palma found and fixed a bug where the right click context menu would send two requests to the server. I was surprised that this was still around since I remember reporting it back in 2008.

#423 Remove explicit render from WikiController#show: Tom Kersten has been working on some new PDF code for ChiliProject and sent in a patch to make sure the Wiki sidebar is rendered explicitly. It isn’t a bug unless you’re hacking on the WikiController… which Tom Kersten is!

#409 [AAJ] Check that bugfix 784bbccf was merged: as part of the major acts_as_journalized system, we’ve had to go back and do some more code reviews of recent changes. This bugfix was one of them and helps to make sure that the tests are testing Custom Fields too.

#416 Refactor watcher_tag and watcher_link to use css selectors for the replace action: Gregor Schmidt did some work on how issue watchers work. When an issue is watched, several elements on the page have to be updated (top Watch star, bottom Watch star, and the sidebar list of watchers). Gregor’s code made this simplier, instead of having to list every thing to update we can now use CSS selectors to update everything. I was happy to review and commit this patch because I was noticing how ugly the watching code was getting and I have a bunch of my own features to include soon.

#345 Entering large numbers for ‘Estimated Time’ fails with ‘Invalid big Decimal Value’: way back on Redmine we had to add a security patch to handle large numbers. Since the security bug was fixed on Rails 2.3.3 the patch wasn’t needed anymore and I was able to remove it.

#350 Setting model should use Rails.cache instead of class variable: Jan Schulz-Hofen proposed a change to how ChiliProject caches it’s Settings. Before this it used class level variables to cache them which had a bunch of problems. Jan, Felix, Gregor, and Holger did some work on it to convert the cache to use Rails’ built in caching. This might be a slight performance improvement but it will also let us using other caching systems like memcached automatically. I reviewed the code and committed the first set of changes, Felix is looking at making a few tweaks to it before 2.0.0 is released.

1.4.0 release: The biggest news of all was that I packaged up and released ChiliProject 1.4.0. This should be the final stable release of the 1.x series, we are getting 2.0.0 ready to release next. (Since Friday ChiliProject 2.0.0RC1 was also released).

Things are staying quite busy in ChiliProject for me. Been doing a lot more code reviews and patch checking than normal while everyone is getting these releases out.

Open Source Friday #3 – acts_as_journalized bug fixes 2011-05-20

Last Friday was a busy one for ChiliProject.

I started the day off with our second Development Meeting 1 where we discussed

  • the state of ChiliProject 2.0.0
  • goals for our 3.0.0 release
  • what to start posting on the ChiliProject blog

It was a great meeting like always and we were able to stick to the 1 hour time frame. I would like to get some more people involved next time, especially to help work on our Teams a bit.

Wrap long text fields properly in PDF exports: Hugo Ferreira discovered a bug in our PDF exports where long fields weren’t wrapping correctly (like the issue subject) and would disappear off of the page. I was able to review his pull request and merged it into the master branch to be included in ChiliProject 1.4.0.

Replace favicon: Muntek noticed that we never changed the favicon since we forked from Redmine. I already had the image in the theme we are using on ChiliProject.org so it was an easy addition.

Last week I performed the merge of acts_as_journalized into unstable. While the code was almost done, there are a few bugs and areas that needed to be reviewed before it’s release. One of those was found and patched by Felix, where the Activity event types weren’t correct. It was a simple fix once it was found and should also make it easier to show different changes in the Activity stream better.

Another major change I did last Friday was to review all of changes to the Journals from acts_as_journalized to make sure we didn’t miss any bug fixes or features from the merge. There ended up being quite a bit missing, which is probably because acts_as_journalized was split off from the core code a long time ago and I’ve done some refactoring in there since then. The main changes were:

  • removing Journal diffs
  • fixing the Atom feed
  • adding the Journal preview
  • adding the missing tests

So last week’s Open Source Friday was a productive one. Not really any new features, just a bunch of bugs fixed. I have to say, I’m happy that we are doing so many code reviews now. Half of these bugs wouldn’t have been found without reviewing what each other is doing.

Open Source Friday #2 – acts_as_journalized and bundler – 2011/05/13

Forgot to post last week’s Open Source Friday so I’m doing it now.

I’m still busy working on ChiliProject’s 2.0.0 release. This is a major milestone for the project and everyone has been working hard on it.

Review and Merge acts_as_journalized: Last time I did a large code review on acts_as_journalized and so on Friday I performed the actual merge. It’s currently in unstable only but I’m already seeing a huge improvement in some internal APIs. acts_as_journalized will also let us add history and audit support to anything in ChiliProject. Once history is added to other objects, administrating an install should become a lot easier because you’ll be able to have a record of who did what. I’m thinking the first two things that would benefit from this sort of tracking is the Settings and Projects. Best of all, it should work with plugins too so plugin data can be “historized”.

Bundler: I did the final review of Gregor Schmidt’s switch to bundler for ChiliProject. This was my first experience with bundler since the upgrade fiasco several months back. I have to say, I’m very happy with how bundler works now. Hopefully this will make it easier to install and package ChiliProject from here on out.

This Friday (today) I’m wrapping up some of the last ChiliProject 2.0.0 bugs. We are going to try to have an early release of 2.0.0, possibly an RC release next week (end of May). Then I’ll be able to start including several dozen major features I’ve had pending for months (years).