Monday, December 12, 2011

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.

No comments:

Post a Comment