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 this.name. Why?

• Imagine this:
• Application.cfc
  • This.name=parent
  • 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:

• A Beginngers Guide to ColdFusion and CFCs
• Local
  Scoping
  
• Var-Scoping, Private, and Public Data in CFC’s
• Thread safety example: Var scope your loop index in ColdFusion CFCs!
• Var Scoping ColdFusion Tag-Specific Variables

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

VarScoper