Formal Code Reviews vs. Automatic Tools

One of questions I always get when talking about code reviews, formal or otherwise, is “What do you think about automatic code review software like CodeCop or varScoper?”

First off I’ll say, I love and use varScoper. Second, I like the idea of CodeCop, but I’ve never been good enough at writing regular expressions to craft my own rules well enough to use it. Those points being said, as great as these tools are, they do not even come close to eliminating the need for formal code reviews.

Automatic testers are great at finding small discreet known issues:

  • Did you var scope every variable in ever function?
  • Did you make sure not to use relative url’s in cfschedule?
  • Did you use cfqueryparam on all of your user input driven queries?

But they can’t figure out bigger picture issues like:

  • Is this function named “computeCosine” actually returning the sine?
  • Is there sufficient validation occurring on form posts before they are being processed?
  • Is this function doing something that a built in function could be doing better?

In addition to the hard to compute issues above, there are the unknowns. Until someone discovered there was an issue with using relative url’s in cfschedule there was no rule written in the code review tool. It takes human eyes to discover these issues, either during a review, or during bug fixing. Only after they are discovered can they be codified.

To put more succinctly and in a metaphorical manner:

Automatic code review tools are spell checkers. Formal code reviewers are editors. Spell checkers and even grammar checkers don’t prevent you from writing perfectly spelled and constructed crap. You need an editor to prevent that. Just as code review tools don’t prevent you from writing crap code that adheres to all of their rules.

So use automatic tools before a formal code review, just like you spell check an email before you send it out. But don’t rely on an automatic tool as your last line of oversight.


Reminder about ColdFusion Unconference

Hey! There’s a ColdFusion Unconference going on at Max this year.

Oh yeah and by the way, I’m giving two talks at it.


Selling Professional Development at a Resistant Shop

Choosing to use tools like Subversion, ANT, and frameworks is the easy part. Getting your co-workers to join in the fun, that’s the hard part. This session will leave you with a selection of tools and techniques to bring your co-workers on board.

Tuesday November 18, 2008 3:00 – 4:00pm at TBA

Formal Code Reviews

Everybody talks about Formal Code Reviews, but there are few resources for figuring out how to actually do one. This session will talk about the issues surrounding code reviews. And, if I’m daring enough, I may just do a live code review, with the audience reviewing a small block of code.

November 19, 2008 9:30 – 10:30pm at TBA


I hope to see you there.

Windows 2008 NLB with 2 NICs

I ran into a problem with our standard configuration for web servers, and couldn’t find the real solution documented anywhere, so here it does.

We run our ColdFusion servers on dual node Windows Network Load Balancing (NLB) servers running in IGMP multicast mode. We run it on machines with two network cards. The cluster address is on one NIC and the nodes answer on another. It’s the configuration we’ve come to like after years of working with NLB and port flooding and other anomalies.

I’m installing a new production NLB cluster for Knowledge@Wharton. To future proof it, and avoid upgrades down the road, I’m going with ColdFusion 8 64 bit on Windows 2008 64 bit. I ran through the configuration steps that I always take setting up an NLB cluster, and it worked… sort of. See the cluster address answered if you called it from another host on the subnet that the cluster was installed on. However, if you were off subnet it didn’t answer. This is suboptimal for a web server.

I worked with our networking team, and they figured out (from this post: ) that if you added a gateway to the cluster NIC, it would work. This is counter to the way NLB has worked before, and generally not best practice. So we opened a support case with Microsoft. After a few tries, I finally got an engineer that was an expert on NLB in 2008, he had the exact cause and solution for this problem: by default IP Forwarding is not enable in Windows 2008. This is the feature of Windows networking that, in the context of NLB, allows responses to requests sent to one NIC to be routed out the other. It’s fixed by using one specific command line option.

(Make sure you are using a command prompt with administrative privlidges)

netsh interface ipv4 set int “[name of the NIC]” forwarding=enabled

That’s it.

Using ColdFusion and SVN to Create Release Notes

I was talking last week about using XML to act as an intermediate step in building your documentation (Automating Documentation and Automating Documentation Part 2). It dawned on me that I could also share my technique for building release notes.

I use Subversion. I’ve gotten positively anal about commenting when I make changes. So if you were to take the history of my SVN commits, they make pretty decent release notes. The trick is to grab them and automatically turn them into documentation.

SVN makes this pretty easy. It takes one command to grab all of the changes. It takes one switch to make the change export as XML. Once that is done, manipulating the content in ColdFusion is a breeze. I do it using <cfexecute> and svn.exe, below is my code:

<cfset svn.wc = “[path to svn working copy]”

<cfset svn.exe = “[path to svn executable]”

<!— -v = verbose –xml outputs it as xml —>

<cfset svn.arg = “log #svn.wc# -v –xml”

<!— get contents of SVN history —>

<cfexecute name=“#svn.exe#”




<cfset changes = XMLParse(changes) />




<!— lxml = LoopLocalXML (shorted for display) —>

<cfset lXML = changes.log.logEntry[i] />









<p>Files Effected</p>




<!— lPaths = LoopLocalpaths —>

<cfset lPaths = lXML.paths.path[j] />











In my build process, I use <cfsavecontent> and <cffile> to write the content to a file, and then use ANT to call the CFML page that creates the release notes – voila, release notes are now part of every build, with no extra work on my part.

ColdFusion and ODBC Agent

This sent a co-worker down a wrong path yesterday, so I thought I would blog it in case it tripped anyone else up.

There was a problem with one of our SQL servers yesterday. (It was down a long time during a routine patch and reboot due to extra stuff in a Microsoft Tuesday patch.) In trying to troubleshoot the issue, one of the administrators saw errors in the event logs coming from the ColdFusion ODBC Agent.

We are using default driver for Microsoft SQL Server in the drop down on the Data Sources page. Therefore, we were using JDBC driver and weren’t impacted by the ODBC error. The error message was a red herring. It took focus away from the real problem, namely that the database server hadn’t come back online yet.

Going further, as far as I can tell all of the defaults on that page are JDBC drivers, with the exceptions of “ODBC Socket” and “Microsoft Access”. According to this Damon’s comment on this blog post, the “Microsoft Access with Unicode” doesn’t even need the ODBC driver. So I would say, unless you are doing coding against Access databases, or a known ODBC only product, you probably don’t need to install the ODBC services; especially since you can always install them if you need them.

I’d be interested to know if anyone violently disagrees, or if this has been said authoritatively somewhere, and I just not up on my Google-Fu.

On Code Reviews… again

I gave a presentation on code reviews to the Philly CFUG a few weeks back. Then my host died, and I never posted this. I’ll try and get the presentation up on the site in the next few days. But two questions arose that I figured I would discuss here:

  1. Do you check the code after the review to make sure developers have made the changes for which the team has asked?
  2. Can developers request a code review?

Both of these speak to a subtler issue. Does your team want to be adequate or excellent? What about each member, what do they want? The answer to this question will inform the answer to the previous ones.

Now this isn’t want of these things where I’m using reverse psychology to encourage what I want others to do — “Well if you want to be only adequate…” Look, not everyone can be excellent. I don’t want to get into a whole discussion on where someone brings up “Free Markets” and someone brings up Harrison Bergeron and then we all get angry at each other. My brief statement on this is that there is plenty of value contributed by people who are only adequate.

But it’s going to make a difference with a whole lot of things, including code reviews. Those that want to be excellent welcome criticism, because the idea of their product being flawed in any way is appalling to them. It’s more appalling than letting other people find those flaws. But really, they like to show off what they’ve done, and want to be acknowledged by their peers for it too.

On the other hand, those that only want to be adequate don’t care if something has flaws, as long as it works. They may try and avoid reviews, but mostly they’ll tend to be indifferent to the whole process, and will only do it if they are mandated. Unfortunately once it’s mandated it becomes a perfunctory step for them, a hoop to jump through. Some of the more militantly adequate will say things like “Don’t bother fixing it; let’s see if we can sneak it past the code review.”

So, if you have group of developers striving to be excellent, they absolutely will ask for their own reviews (but you might need to let them know they are available) and you won’t have to check them afterwards. On the other hand, if your team is on the other end of the spectrum, you might just have to mandate them. Perfunctory code reviews while painful are better than none at all.

As for double checking the code, it comes down to trust. Do you trust your teammates? I say err on the side of trust. Code reviews tend to ruffle a few emotional feathers to begin with. To big brother people afterwards would just add insult to injury. Trust people until they give a reason not to trust them. Obviously if you are doing work with sensitive data you may want to evaluate this on your terms.

But if people have violated your trust, you’re going to be glad you followed the rest of my advice on code reviews. That way you’ll have page names and line numbers by which to judge whether or not they kept up their end of the bargain.

This tension between adequate and excellent effects many other aspects of your team. I’m thinking you may want to look at this for more than just code reviews. I figured this would be a good place to sneak it in.

Top Code Review Issues

I figured I would blog about what I’ve been seeing during code reviews lately. Maybe someone else has been seeing too, or seeing other issues. I’ve omitted problems that deal with our shop’s internal best practices, and tried to stick with things that would apply more globally. If I over applied something let me know. It’s a long one, so more after the jump.

5. Improper implementation of Application.cfc inheritance

Application Inheritnace is very useful for application and sub applications development. However there is a gotcha to remember here. If you are going to override application variables that you set in onApplicationStart, they you need to give the application.cfc its own Why?

  • Imagine this:
  • Application.cfc
    • OnApplicationStart
      • <cfset application.title = “Parent” />
  • Child/application.cfc extends=”../application”
    • OnApplicationStart
      • <cfset application.title = “Child” />

In this case, assuming the application is not started, going to a page under the child will trigger onApplicationStart. But since they have the same application name through inheritance, going to any parent page will not trigger OnApplicationStart, and they will be stuck with the application title of “child.”

The easy fix here is to always give children a different application name, only omitting it when you have a specific reason not to.

4. Improper Access on CFC methods

The four access types of CFC methods tend to cause people some confusion especially when they are learning the whole CFC thing. They each have different implication for application security.

I’ve pasted a more in depth guide below, but for the most part you probably shouldn’t set access=remote. Even if you are writing a webservice, only the exact method you are calling needs to be set to remote, so don’t set all of the methods called from your webservice to remote.

  • Private
    • Only accessible by other functions in CFC
    • Used for sub functions only relevant to CFC
  • Package
    • Only accessible by code in same folder
    • Any sort of organization makes this almost useless
  • Public
    • Accessible by any code on the server
    • Sounds scary, but relatively safe on a dedicate server
    • Use with caution on a hosted box.
  • Remote
    • Used for webservices.
    • If building webservices these should be accompanied by some security or logging.

3. Inappropriate use of a Query of Queries

Query of Queries have many legitimate uses. They include but are not limited to:

  • Performing SQL syntax queries on query objects returned from non-SQL operations. (Ex. <CFLDAP> <CFDIRECTORY>)
  • Joining the results of two queries made from two different sources (Ex. Joining MS SQL and Oracle data, or SQL and LDAP data.)
  • Providing multiple views to the same data on the same page request. (Ex. Showing a chart of data sorted multiple ways on the same page.)

However, where they get deservedly shunned is when they are reproducing functionality that can be completely handled in native SQL. In my experience, this is often (but not always) done by developers who know Cold Fusion very well, but not advanced SQL, therefore when they can’t get the problem solved in SQL they fall back on Cold Fusion to do what they need to get done. (At least that’s what I was doing when I got yelled at for it.)

In many cases, it takes less time to post the SQL request to the database server and get the response back then it does for Cold Fusion to process the request on an in-memory query. It’s counter-intuitive, but such is the power of indexing and SQL-based set operations. Case in point, when I was designing a dictionary check for a password change page, it made sense to load the entire dictionary into an application variable, and then do a query of queries on it. The process took 3000ms, or 3 seconds, which is slower than a request really should take. The same request against the database server took half that. Ultimately through indexing and query tuning, I got the process down to 100ms.

Your mileage may vary, especially depending on the size of the dataset, but the point remains that you should let SQL be SQL and Cold Fusion be Cold Fusion.

And by the way, I’m not being original here, other people have said this:

ColdFusion is Not a DBMS

2. Fake Email addresses in error or reporting pages

If your application sends email, those emails should be sent from a legitimate, addressable email address. (Whether or not the email goes to a list, a mailbox, or some other place is not a factor.) There are a couple of reasons for doing this:

  • It is not inconceivable in the future that your organization, hosting provider, or your user’s email host will start rejecting messages from invalid addresses due to spam considerations.
  • Any bounces generated from these messages will never get back to you.
  • Users responding to these messages will get ungraceful bounce responses.

Of these reasons, 1 is probably the most important as spam or virus concerns have forced mailing hosts to make quick decisions like this without consulting others, due to the severity of the issue. But this is one of those easy to fix low hanging fruit issue.

1. Lack of Var scoping in CFC methods

This is a pretty familiar issue to everyone. I don’t want to belabor it. As a matter of fact I’ll just link to a bunch of posts about it:

Additionally there is a tool you can use to help programmatically attack this problem:


Running a ColdFusion Shop Part 4

I received an email today, that reminded me of a topic I wanted to throw up on my Running A ColdFusion Shop series.

The email:

Subject: Mail Queues Need Attention

Body: Hoth Spool could be backed up.

query – Top 1 of 1 Rows
04/17/2007 05:43:54 PM C:CFusionMXMailSpool Mail22186.cfmail 719

To unblock spool:
Drain stop node(wlbs drainstop)
Restart Cold Fusion Service.
Restore node (wlbs start)

Make sure before doing this that the node is actually backed up.
One or two messages in the queue does not a blockage make.
But depending on the number of items and length of their stay, you can make the determination.

What this email reminded me to say was: DRY isn’t just for code.

We had this problem a while back. Every once in awhile, the mail queues on a random one of our ColdFusion servers would back up. The mail would remain stuck until the server was restarted. Developers and users started getting pissed about their application email being delayed.

The short term solution was to check the queues every once in awhile. Once the problem stopped occurring, we stopped checking… until it happened again. We looked for a hotfix, to no avail. We did this a couple times, and each time we got burned when we stopped being vigilant.

Finally I said “Screw it; I’m scripting a solution to it.” I started checking to see if files were in the spool directory of all of our servers. Then I had to make sure that the files had sat there for a little bit instead of just having been written there. Finally I had to send an alert with the needed fix.

No big deal, it’s not advanced programming, it’s not even particularly good code.

The point here is that it was running in a scheduled task… since I wrote it in 2005 (and restarted it earlier this year.) We had this problem again today, and nobody outside of my team noticed.

So what’s the point of this besides a little bragging? Don’t do things by hand that you can automate and forget about, or DRY isn’t just for code.

Running a ColdFusion Shop 3

For Reference: Part 1 and Part2.

It’s been awhile, but I’ve finally got more to say about all this.


In the last one of these, I told you to back up your system. Maybe not directly, but it was there. Okay, so I didn’t say it. I’m saying it now. Backup your machines. Do a tape backup, export to an external drive, do anything. Just backup your machines.

Backup Configuration Files Separately

Independent of a backup solution that takes care of your whole system, I would also recommend backing up configurations to separate location, in their current format. Meaning, don’t put them on a tape, don’t ship them off site. Just keep them somewhere you can access them. Why? Coldfusion is pretty light. CFML sites are comprised of pretty small files. Odds are, if you have a systems failure, it very well may be faster to rebuild a system from scratch then it will be to get a system backup restored and working. Rebuild your server, reinstall your webserver, and reinstall ColdFusion, all while getting the CFML files from backup. Then use configuration files you’ve stored elsewhere to restore both your webserver and Coldfusion to its old state. By this point, the CFML files should be out from a restore.

There are other uses for these files. You can use this method to clone your boxes, for clustering for example. Additionally, the ways they are broken up make it easy to share certain areas of configuration between machines. The most useful of these is neo-query.xml, which has all of your datasources on it.

Anyway, you probably want to know where all of these are:

  • ColdFusion configuration information is stored in xml files, in [CfusionRoot]lib.
  • IIS XML configurations can be exported from the IIS Manager
    • Right Click website
    • Choose “All Tasks”
    • Choose “Save Configuration to file.”

I’m sure that Apache has something to handle this too. But I don’t have any experience with it.

Backup Certificates

If you are using SSL certificates, back them up. They are a pain in the ass to manage. If you lose one without a backup you have to depend on the Certificate Authority to get your SSL service back up. Better to just have them ready to go in case of a problem.

Again I can only speak authoritatively about IIS, but:

  • Go to IIS Manager
  • Right click your SSL secured site
  • Choose “Properties”
  • Go to “Directory Security”
  • Under “Secure Communication” hit the “View Certificate” button
  • Choose “Details” tab
  • At the bottom “Copy To File”

That will bring up the “Certificate Export Wizard.” I usually choose the default options, with one exception. I choose “Yes export the private key.” It’s less secure, so you want to make sure you store them somewhere safe, but you’ll be able to do this again with a certification created from a backup with the private key.

That’s all I got this time around. Sorry it’s all backup related. Nothing happened; I was just collating tips for building a new Coldfusion box in our environment, and realized that these were all pretty important to doing that

On Code Reviews

I know I said in a previous post, that enough has been said about code reviews, but I’m writing about them. I may have to do more code reviews in the near future, so it was a good time for me to gather myself and think of all the principles I should keep in mind. Also, if anyone else has any thoughts on the matter, I would love to hear them.

Define Your Goals

This seems a little weird at first. “Our goal is to write good code.” Well yes and no. The group that I manage is in charge of administering the SQL, ColdFusion and IIS boxes. My personal goal with code reviews is to ensure I don’t get paged at 2am because our servers are down. I primarily look for performance and security issues. Another member of the code review team might manage a team of programmers; they might be looking at compliance with an accepted set of conventions, because their goal is to make sure that when changes need to be made, anyone on the team can quickly make them. Still another person on the code review team might be focused on usability as their goal is to comply with government regulations.

It’s okay that there might be more than one goal. Just figure out what your organization’s goals are, and shape your team’s goals accordingly. My team’s goals are:

  1. To protect shared resources from performance problems due to inefficient code.
  2. To show off new and interesting techniques
  3. To learn from each other’s experiences

Define Standards

Now that you know what your goals are you need to come up with what the rules are. I start with the Adobe CFMX Coding Guidelines. I look at performance and security issues, and then start building out rules from there. Then I collect tidbits from other online sources, and start lumping them all together.

I need to say, before I started any of this, I read my copies of the ColdFusion MX 7 Web Application Construction Kit and Advanced ColdFusion MX 7 Application Development. These are so fundamentally important to writing good ColdFusion, that we don’t even bother telling people to read them. You start working for us, you get handed copies of them.

Publish Them

This seems to be a no brainer, but people really get annoyed when they come to a code review and are told they’ve violated our super secret standards. Now in our mind, they’re never secret. Everybody who is reading the blogs know what techniques are out of favor now right? Well no, because not everyone who codes in your organization keeps up to date with the ColdFusion blogosphere. So you need to tell them what they standards are. We do this by including a blog in our development center that has a category named “Best Practices.” We add to it as techniques or situations come up. We then force our developers to see the blog, because the rest of our development tools are on the same site.

Give Directions

We very tell people the following:

  • Make sure your application conforms to current best practices
  • Prepare CFML, HTML, CSS
    • Make a local copy of your code. Feel free to include stylesheets, but don’t bother with images.
    • Remove database, or other passwords from code.
    • Remove any unused templates
    • Clean any commented code out of your applications
    • Remove development diagnostic code like <cftimer> or <cfdump>
    • Remove database, or other passwords from code.
    • In short, code should be ready to go to production when reviewed.
  • Prepare SQL
    • All SQL going against a database server must be called in a stored procedure.
    • Use Query Analyzer to properly index tables. [Link to tutorial]
    • Use Enterprise Manager to generate scripts for all tables, including indexes. [Link to tutorial]
    • Use Enterprise Manager to generate scripts for all stored procedures. [Link to tutorial]
  • Prepare Flow Chart
    • Use Visio or some comparable program to chart the flow of your application.
    • Visio can be obtained from [Link to Central Installs location]
  • Zip up all the collected files.

You may require other things. But make sure people are told what those things are.

Read the code before the review

Nothing irritates me quite as much as a reviewer who insists on not reading the code ahead of time. To force others to wait because you say “hold on” as you flip through pages is incredibly disrespectful to you co-workers.

Some people print out all of the code, and review directly on the paper. With the size of applications these days, this is becoming more and more unwieldy. I’ve taken to using my two monitors and putting up code in one window, and Microsoft One Note in the other. That way I can just record my thoughts, or copy and paste code. Additionally, I don’t waste paper.

Make clear and concise recommendations with references to specific lines of code. Here is a sample from one of my reviews:




Why do you raise the logout action to an application variable? Is there a good reason for doing so?


Line 319-326

I really like this bit of logic.


Line 61

This is a bit confusing: either the object you are creating either exists throughout the entire CFC (which is only one function for now, but could grow at some point.) so it should be in the variables scope and init when the cfc init’s. OR it only exists in this function in which case it should initialized with a var.


Line 22

CompareNoCase is prefered over eq

1 eq 1 will always be true. So this conditional will always be true so get rid of it.


Line 23

Using the error.mailto is preferred over using a hardcoded address.


Function frontier_calc

I feel like this function might be a little hard to maintain and troubleshoot.

Additionally it has explicit steps within it. It could be encapsulated a little further to make troubleshooting and debugging easier.


  • Must Change
  • Suggested Change
  • Stylistic Tip
  • Curiosity, Not Criticism
  • Comment

Notice also that we make allowances for several levels of severity. There are things that must be changed. But we also place an appropriate priority on less important changes so developers against a deadline know what they have to do, and what they can negotiate with us over.

The Review

The developer presents their application. They tell us what its purpose is, any specific challenges they faced, or what problem they are trying to solve. Then we usually go through the code technology by technology. (So Flex code, then ColdFusion code, then SQL, then XHTML/CSS etc.) The team gives our feedback, and then discuss any points of contention. (I think something is the best way to do it, but another review disagrees, and the developer has a specific reason for choosing to do it in a way other then what we think is “best.”)

Keep it courteous. Keep it respectful. And don’t forget to praise the good things you find.


The developer should make changes to their code based on the review. The reviewers aren’t done yet though. Was anything “widely accepted” as best practices new to a developer? Then it should be posted with your best practices; even if you have to repost it. You may also have to sit down with your fellow developer and help them learn a new technique.


Code reviews are a necessary part of running a good programming shop. No one should be exempt; even the makers of the standards. They can be painful, and ego bruising, but with a little preparation, the negatives can be downplayed, and the positives allowed to shine through.