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().