Scrutinizing another project code base: what to look for

So when you're evaluating an unfamiliar project's codebase, make sure to be careful and not be deceived by how it first looks. On the current project we're working on the code looked like it had a good architecture to begin with, but after working with it for a few weeks you begin to notice some things that are just weird.

Here's some things to watch out for (keep in mind we have an ASP .Net MVC 2 project):

• Controller logic mixed into the view layer (this is a terrible sign). Not very common in this project but I've seen plenty of it.
• (Off that bullet) Too much C# code in the view layer that's not really controller logic, but stuff like loops, if blocks, etc. that render different HTML elements are just ugly. 
• Embedded JavaScript code inside aspx files: In my opinion this is sloppy and pollutes the markup with JavaScript code. What's even worse is JavaScript code that also contains C# code in it!
• Large JavaScript files. JavaScript code should be treated like your other code -- this includes having some kind of separation to give a modular structure (makes it unit testable too) 
• Code duplication. This is a big one: why is the code being duplicated? A severe case in this project is two JavaScript files containing the same code but in different places. They even had the same typos! A really annoying case of this is when you have three classes with the same name but in different namespaces.
• Doing too much in the data layer. Having too much stuff (such as branching logic etc.) in the data layer makes it hard to unit test the bugs that will pop up from it.
• Commented out code: This is not a red flag but should still be a warning sign. Why is there code that's been commented out? If it's under source control (and it better be) you already have a copy of it, if not then why isn't there an explanation of why it's been commented out?
• Swallowing exceptions: This is my favorite because often to the user it just looks like their button doesn't work, when in reality an exception was caught, swallowed, and the action just didn't proceed.

So, I've seen all of these things in the project we're on now and this is based on that experience. Your results may vary.

Code review, refactoring, and more...

Today I want to tell you about ensuring that code is maintainable.

Now, in our project we have a Servlet that reads in requests for graph plotting using jqPlot, and it sends these requests off to a GraphsManager which is responsible for making the data into a nice jqPlot-compatible object that can be JSON serialized and sent off to the client.

The code in GraphsManager is a little hairy -- this is because it makes heavy use of generics and some reflection to ensure that it's very general and can work with many different data sets.

Even though our code base is small right now, it still has the ability to become unmaintainable to anyone who didn't write a particular piece of code. So we want to avoid that by having regular code reviews where we share the pieces of code we've written, go through what it's doing (and why), and refactor it to make it more readable and maintainable.

In going through GraphsManager, even simple changes like replacing confusing if conditions with a helper method that has a very clear name, we can make the code a lot more readable for someone that didn't write it. We also discuss why the particular approach was taken, its pros/cons, and possible alternative solutions.

Anyway, we did this at our meeting the other day and I felt it was really constructive. In fact, I'm thinking about refactoring it soon using an entirely different approach that will make it a lot nicer. It also helps that GraphsManager has an extensive series of test cases that will ensure we don't break anything.

So the moral of this story is: Code review is good, and going through your code with someone will reveal its flaws and possible improvements you can make to it.