From time to time in my job I'm asked to do code reviews. I love em. I get to tear apart someone else's code and pretend like I never make mistakes. It's perfect. I don't often review code over email as it's typically a big job, and I don't want to encourage folks to send me reams of code (well, not unless you have a budget ;), but as this was one simple file I thought I'd help the user out, and then share the results here. Is this kind of post useful? Let me know. I think it is as it shows real life production code - but again - do let me know. (I also know that people are still struggling to grok application.cfc - so I figure the more examples the better.)
So first off - here is the code as it was sent to me. I removed a few lines that were repetitive and not useful to the review and I changed a few values so as to not reveal any personal information:
<cfcomponent output="no">
<cfset this.name = 'xxx'>
<cfset this.applicationTimeout = createTimeSpan(0,0,2,0)>
<cfset this.clientManagement = true>
<cfset this.sessionManagement = true>
<cfset this.sessionTimeout = createTimeSpan(0,2,0,0)>
<cfset this.setClientCookies = true>
<cfset this.setDomainCookies = false>
<cfset this.scriptProtect = false>
<cfsetting showdebugoutput="false">
<cffunction name="onApplicationStart" returnType="boolean" output="false">
<!---set some variables for DB, cfmail, and common emails used throughout site--->
<cfscript>
application.dsn = "xxx";
application.dev = "xxx-dev";
application.root_url = "http://www.cnn.com";
application.rootmap = "/cnn";
</cfscript>
<CFIF CGI.SERVER_NAME IS NOT "www.cnn.com">
<CFLOCATION url="http://www.cnn.com#CGI.SCRIPT_NAME#" addtoken="no">
</CFIF>
<!---set path for Doug Hughes Reactor--->
<CFSET Application.Reactor = CreateObject("Component", "reactor.reactorFactory").init(expandPath(".") & "/reactor.xml") />
<!---set paths for other frequently used components-the memLib is the one I was talking about yesterday--->
<CFSET Application.Boo = CreateObject("Component", "model.util").init() />
<cfset APPLICATION.memLib = CreateObject("component", "model.services.memberservice").Init()/>
<!---set structs to work with Reactor files--->
<CFSET application.relationshipcodes = structnew()>
<CFSET structinsert(application.relationshipcodes,"", "")>
<CFSET structinsert(application.relationshipcodes,"1", "Head Of Household")>
<CFSET structinsert(application.relationshipcodes,"2", "Spouse")>
<CFSET structinsert(application.relationshipcodes,"3", "Child")>
<CFSET structinsert(application.relationshipcodes,"4", "Other")>
<CFSCRIPT>
memberDOB = createDate(year(now())-12,month(now()),day(now()));
gateway = Application.Reactor.createGateway("State");
query = gateway.createquery();
where = query.getWhere();
where.isIN("state","country_id",'182,267');
Application.stateQuery = gateway.getByQuery(query);
</CFSCRIPT>
<cfreturn true>
</cffunction>
<cffunction
name="OnRequestStart"
access="public"
returntype="boolean"
output="false"
hint="Fires when prior to page processing.">
<cfif StructKeyExists( URL, "reset" )>
<cfset THIS.OnApplicationStart() />
</cfif>
<cfreturn true />
</cffunction>
<cffunction name="onError" returnType="void" output="true">
<cfargument name="exception" required=true>
<cfargument name="eventName" type="string" required=true>
<cfmail to="#application.web#" from="#application.web#" subject="Error in #application.applicationName#" type="html">
<cfoutput>
<h2>An error occured!</h2>
<p>
Page: #cgi.script_name#?#cgi.query_string#<br>
Time: #dateFormat(now())# #timeFormat(now())#<br>
</p>
<cfdump var="#arguments.exception#" label="Exception">
<cfdump var="#url#" label="URL">
<cfdump var="#form#" label="Form">
<cfdump var="#cgi#" label="CGI">
</cfoutput>
</cfmail>
<cfoutput>
<h2>We are sorry...</h2>
<p>
An error has occured while processing your request. Please do not be alarmed.
We have sent an error message to the monkeys who coded this site and once they
wake up from their four hour naps, they will attend to this bug.
</p>
</cfoutput>
</cffunction>
</cfcomponent>
And here were the notes I wrote back to him with. These are in no particular order.
I'm not a big fan of structInsert. It's not bad - just extra typing:
<CFSET application.membercodes = structnew()>
<CFSET structinsert(application.membercodes,"1", "Active Member")>
<CFSET structinsert(application.membercodes,"2", "Prospective Member")>
I'd switch to:
<cfset application.membercode[1] = "ActiveMember">
Snippet from onApplicationStart:
<CFSCRIPT>
memberDOB = createDate(year(now())-12,month(now()),day(now()));
gateway = Application.Reactor.createGateway("State");
query = gateway.createquery();
where = query.getWhere();
where.isIN("state","country_id",'182,267');
Application.stateQuery = gateway.getByQuery(query);
</CFSCRIPT>
The variables above (except for the Application variable) should be var scoped. The Application.cfc follows the same rules for var scoping as any other normal CFC or UDF.
<cfif StructKeyExists( URL, "reset" )>
<cfset THIS.OnApplicationStart() />
</cfif>
This is bad. Remove the This. When you are in a CFC and do
this.x()
You are doing an "outside" call to the CFC. So if x was access=private, it would fail. Just do x().
Archived Comments
I think it helps a lot, although it does open you up to a flood of e-mails. It is a good learning experience to have your code reviewed by someone who quite literally "wrote the book." It is one thing to say you shouldn't do this. It comes in handy to see WHY you should (or should not) do something in a particular way.
That type of commentary looks very familiar. Good to know I've been doing it right.
Are there any tools you use to make reviewing code a little easier? I use Jupiter currently, but I'm always looking for something better.
I'm a bit ashamed to admit it - it never even occurred to me there may be tools to review code. I just use my editor. I have heard of stuff from CompuWare that does automatic reviews.
Do you have a URL for Jupiter?
Ray - my apologies... you're going to hate me going on about <cflocation>, but this:
<CFIF CGI.SERVER_NAME IS NOT "www.cnn.com">
<CFLOCATION url="http://www.cnn.com#CGI.SCRIPT_NAME#" addtoken="no">
</CFIF>
Should be (if you're on CF8 - if not, you'll need to do some cfheader stuff):
<CFIF CGI.SERVER_NAME IS NOT "www.cnn.com">
<CFLOCATION url="http://www.cnn.com#CGI.SCRIPT_NAME#" addtoken="no" statuscode="301">
</CFIF>
I can guess what they're trying to do; that is to avoid duplicate urls:
cnn.com/whatever.cfm
and
www.cnn.com/whatever.cfm
But you need a 301 redirect to achieve this - a 302 tells a search engine that *both* urls exist.
Sorry! I'll not mention cflocation again! I promise...
Here's a brief note I wrote about it back in March. (Shameless self promotion. )
http://www.numtopia.com/ter...
But you can download it at http://csdl.ics.hawaii.edu/...
It might not be a good fit for you, but when you're doing a few reviews a week, it does a good job of saving you time.
@Geoff - no, not at all. I agree and that is a good point!
Thanks for the link Terrence.
Just a thought on the cflocation bit. What is it doing in onApplicationStart? This will only run when the application is initialised. It's not going to achieve much in there is it? Or am I missing the point completely?
And Ray, yes I like the code review idea. I think as developers, we all secretely love the more "cody" posts ;)
Martin - darn good catch! Boy I'm glad I blogged this now.
Ray, didn't you run a contest a while back for an automated code review application? I seem to remember something called CodeCop that looked promising.
Yep, I did. I got 3 entries I think - all of which were pretty darn interesting. I'm pretty sure one of them is up on RIAForge.
With that cflocation, it's pretty easy to write an ISAPI_Rewrite (IIS) or Mod_rewrite script to handle those types of redirects and keep it clear of CF. Typically I have an ISAPI_Rewrite to handle people hitting my sites without www. etc. I usually put all the domains as host headers on the individual site, .com, .co.uk etc etc and then use ISAPI_Rewrite httpd.ini local to the site which tests if the requested domain is the primary domain and if not then redirect it to the primary domain - this handles people hitting sites without the www. or hitting any of the alternative urls.
In our org, our best tool for code review purely on a "what did this person do" matter is trac. It provides awesome source diffs and in our case down to the ticket level so we can see exactly what was changed for a particular ticket(task), It makes looking at 50k lines of code a bit easier to swallow
Sorry if my question is stupid:
I would like to know where exactly you place the application.cfc in the web application.
Not stupid at all Vicente. When you request a CFM file, ColdFusion looks for a file named Application.cfc or Application.cfm in the same folder. If it doesn't find it, it looks in the parent folder, and so on, and so on.
When you refer to this section:
<cfif StructKeyExists( URL, "reset" )>
<cfset THIS.OnApplicationStart() />
</cfif>
This is bad. Remove the This. When you are in a CFC and do
this.x()
IMO that is lame, if you are bothering to scope the function call then you will be aware of the function's scope?
IE: VARIABLES.x() for private, THIS.x() for remote/public/package
Shuns - I'm a bit confused. Are you saying that you SHOULD scope the function call and I'm wrong?
Hey ray,
Nah I'm not saying that you should, I'm just saying that I dont think that it is bad if you scope functions. So you would scope private to the VARIABLES scope and the rest of the access types to the THIS scope. That is if you want to.
The point I was trying to make is that I didn't think that it was bad to scope them as you had said.
Shuns - I don't think I understand you. If you do this.foo(), you are essentially calling your method remotely. It's like using an area code to call your neighbor.