What did you learn today? - Code Reviews

Reviewing Databases

Phil Denoncourt — Wed, 21 Mar 2007 15:59:42 GMT

There is plenty of guidance on reviewing code. (Look at CodeFrisk.Com/Guidance.aspx for some links). I’ve found that the one absolutely most critical piece of a traditional application never undergoes any type of review. Most applications live and die by their database, and yet it rarely gets reviewed. If you’re lucky (although you might not think so), you’ll have a DBA check things over, but they are mostly concerned with performance. Maintainability and adherence to standards are not at the top of their list.

I’ve listed some of the things I look at when I review a database:

-Appropriate Normalization

It’s tough to say what is and isn’t appropriate normalization. You’ll know it when you see it. Stuff like having a separate table for the US states or one called Gender is usually a red flag of having gone too far. Conversely, having the same field in multiple tables is usually a sign that normalization hasn’t gone far enough.

--Is all access through stored procedures

Most development shops have a policy that says, “All data access shall be done through stored procedures”. If you have that policy, check to make sure this is being adhered to. An easy way to check this is to deny access to the tables and allow access to the stored procedures.

--Look for premature optimization

I’ve seen times when developers are using Join hints or locking hints in their query. “If I force the query to use a merge join, it goes 5x faster”. Meaning it goes 5 times faster on the development box which has far fewer rows and 3 fewer processors than the production machine. Once it goes the to production machine, there’s a good chance that SQL will decide upon a different plan to execute the statement, making the hint destructive on the production machine

--Are objects secured and scripted

Eventually, when you roll the database into a production environment, access to the database will (or should) be locked down. Is the development environment the same? Are the scripts that you have under source control (you do have the database creation scripts under source control don’t you?) include securing of the object.

--Are updates applied in the same order

I can think of no better way to create an application that suffers from chronic deadlock situations than to have one stored procedure update tables A, B, & C (in that order) and have another stored procedure update tables C, B, & A (in that order). Updates should always be applied in the same order. Otherwise, you’re increasing the chance that a row in A will be locked by one stored proc while waiting for a row in table C locked by the other stored proc. Generally the order is enforced naturally because you have to respect foreign keys, but every now and then I come across this.

--Biblical stored procedures

By biblical, I mean volume, not divinely inspired. I’m referring to stored procedures that use up more than one printer cartridge if you were to print it. Nowadays when you see a large stored procedure, it’s one of two things:

1) Embedded business logic – This seems to be a remnant of client server programming where you were forced to put your business logic in the stored procedure. It’s arguable about whether or the database is the best place to keep your business logic. Most people (including me) believe it’s not. Business logic should be kept in a business logic component that validates, enforces and calculates.

2) Poorly defined schema. Most validations that you see taking place in stored procs could be controlled by using schema constructs like check constraints, triggers, foreign keys and defaults. Put that type of information in the schema where it can be enforced consistently everywhere rather than burdening the stored procedure.

--Does the Development database match what’s in source control

To reinforce this, database schema objects need to be kept under source control. Having a database backup plan isn’t sufficient. Putting the objects in source control allow you to manage changes and releases much more effectively. Otherwise you’ll be struggling to determine which objects have changed in the development database and need to be deployed, and figuring out who added a field to table. Keeping the schema under source control used to be a very manual process, but now tools like Visual Studio for Database Professionals make this painless.

--Cloned stored procedures / Views.

This is very common. When you need data, most developers don’t look to see if there is an existing stored procedure or view that satisfies their needs. They’ll create a brand new one. Then you end up with a database that has 3 different stored procedures that get a customer record by its ID. Now all these duplicated procs have to be maintained. Best prevention for this is to have a strict naming convention for your procedures.

Those are some of the quick patterns I look for when reviewing a database. What are the kind of things that stand out when you look over a database?

Wouldn’t you feel comfortable having your code reviewed by an expert? Go to CodeFrisk.com to see how I can proofread your code at a reasonable price.

Code Reviews

Phil Denoncourt — Sun, 18 Mar 2007 11:18:14 GMT

As a developer, I love to write code. Getting involved with a problem, putting my head down, coming up a solution and getting immersed in implementing it is an awesome feeling.

I also enjoy reading code. I like to look at a set of code and try to understand what the developer was thinking. You can tell a lot about how a person thinks by reading their code. To me, participating in code reviews is a lot of fun.

I originally wrote these articles 6 months ago when I was about to launch a new service to review code (For details, go to http://www.codefrisk.com/). I’m now much too busy to review code, but these articles might be of value, so I’m uploading them now. I’ll be posting a set of articles about code reviews, best practices, coding standards… Here’s the first article in the series:

Coding Standards – How to write a useful Coding Standard document

Coding standards are an agreement in a development team on how the code for an application will be structured. They promote consistency and easier maintenance. A well-written and enforced standards policy will reduce the time that it takes for new developers to feel comfortable with an application.

I’ve worked with many organizations over the years, most of which have some sort of coding standards documentation. The needs of each organization are very different. Some organizations sold their source code, so they needed to make sure there was a very high level of consistency. Others were in regulated industries and needed to conform to various practices. Others were small shops that didn’t see tremendous value in having standards.

The standards document had varying degrees of success. Some negative factors in coding standards I’ve seen are:

Aggressively guarded by one person
Enforcing rules that were necessary in older technologies, but no longer hold relevance (Especially if they were burned by the problem)
Too slim
Too thick
Too strict
Begun, but never completed.
Finished, but never enforced.

I’ve written a few coding standards docs and want to share what I’ve found useful, and what you should try to avoid.

First of all, if you’re the person tasked with writing the documentation, make sure that you involve the rest of the development team as much as possible. You shouldn’t take the Moses approach; climb a mountain and come down with a set of commandments. If the developers on your team aren’t consulted on the coding standards, you’ll see a lot of them not adhering to them. These aren’t your coding standards; they are the teams coding standards.

Recognize that you aren’t writing a document with a stone and chisel. Coding standards must evolve as your team has learned new techniques. It’s also not uncommon to see coding standard docs that never get finished because the author feels the document must be 100% “complete” before they release it. Take an iterative approach and get the document out as soon as possible and continue to refine it. It’s impossible to get it all right the first, second, or even tenth time. You’ll probably find that most code reviews will result in changes to the standards document.

Kick off a discussion for an hour or two and brainstorm with the development team about possible standards. There will be contention. If the team can’t come to an agreement after 5 minutes, table the discussion point for later. If people have time to think about something, they will usually be able to justify or back off on their opinion. The important thing is to limit the time of the discussion in the interests of getting an initial version within the same day. Good developers will talk about coding all day.

Areas to discuss would include:

Naming standards – Camel, Pascal, Hungarian, Dewey Decimal System?
Documentation expectations – XML Documentation, when/where to put comments, class headers…
Design Patterns that are expected to be used – Factories, Observers, Decorators…
Automated Unit Testing strategy – TDD? Tool type, Code Coverage…
Exception handling – Handling, Reporting…
Use of constants/resource files – Yes, No? One resource for the entire project, one constant class?
Complexity limits for methods - > 25 is a lot of branches
Data access methods - Stored Procs, Dynamic SQL, DataSets, ORM, Async, Transaction strategy…
Common components to use – Enterprise Library, Log4net, Infragistics…
Localization strategy - Needed? Roll your own or use .NET framework...
Versioning – Versioning assemblies, strong name, signing, delay signing…
Build strategy – Nightly builds, one solution or many solutions, creating references
!Code Formatting

For the most part try to stay away from talking about code formatting. Formatting standards inevitably come down to one developer’s preference over another. Capable developers will be able to read the code regardless of how it is formatted.

After you’ve had your discussion with the team, hole yourself up in your office/cube and write up the standards doc. Unless you’re a dreadfully slow typist, this shouldn’t take more than 1 – 3 hours.

The important thing is to make sure you get feedback from your team on the document. If they don’t feel involved and disagree with a particular piece, they will exhibit a little bit of passive-aggressive behavior and not adhere to the standard.

The Coding Standards document sets up expectations at code reviews. It is a declaration of what is expected from a developer when they submit code. Without some set of coding standards, code reviews can quickly become counterproductive.

Coding standard docs aren’t hard, aren’t intrusive, and are a valuable resource. When there is a set of guidelines to use when coding, it is much easier to bring new developers into the project. The benefits justify the one day of effort it takes to put one in place.

Wouldn’t you feel comfortable having your code reviewed by an expert? Go to CodeFrisk.com to see how I can proofread your code at a reasonable price.