2
\$\begingroup\$

I just posted a small open source CFML project here db.cfc is a standalone CFC with 472 lines of code.

Here is the execute function in db.cfc. This is just a partial example pulled from the 400+ lines of the code in db.cfc. If you have any suggestions for the rest of the CFC, feel free to post them.

This code is currently optimized for speed so that it is not much slower than a regular <cfquery>. The project doesn't have any unit tests yet.

<cffunction name="execute" returntype="any" output="no">
    <cfargument name="name" type="variablename" required="yes" hint="A variable name for the resulting query object.  Helps to identify query when debugging.">
    <cfscript>
    var queryStruct={
        lazy=this.lazy,
        datasource=this.datasource  
    };
    var pos=0;
    var processedSQL="";
    var startIndex=1;
    var curArg=1;
    var running=true;
    var db=structnew();
    var cfquery=0;
    var k=0;
    var i=0;
    var s=0;
    var paramCount=arraylen(variables.arrParam);
    if(this.dbtype NEQ "" and this.dbtype NEQ "datasource"){
        queryStruct.dbtype=this.dbtype; 
        structdelete(queryStruct, 'datasource');
    }else if(isBoolean(queryStruct.datasource)){
        this.throwError("db.datasource must be set before running db.execute() by either using db.table() or db.datasource=""myDatasource"";");
    }
    if(not isBoolean(this.cachedWithin)){
        queryStruct.cachedWithin=this.cachedWithin; 
    }
    queryStruct.name="db."&arguments.name;
    if(len(this.sql) EQ 0){
        this.throwError("The sql statement must be set before running db.execute();");
    }
    if(this.verifyQueriesEnabled){
        if(compare(this.sql, variables.lastSQL) NEQ 0){
            variables.lastSQL=this.sql;
            variables.verifySQLParamsAreSecure(this.sql);
            processedSQL=replacenocase(this.sql,variables.trustSQLString,"","all");
            processedSQL=variables.parseSQL(processedSQL, this.datasource);
        }else{
            processedSQL=replacenocase(replacenocase(this.sql,variables.trustSQLString,"","all"), variables.tableSQLString, "","all");
        }
    }else{
        processedSQL=this.sql;
    }
    if(this.disableQueryLog EQ false){
        ArrayAppend(this.arrQueryLog, processedSQL);
    }
    </cfscript>
    <cftry>
        <cfif paramCount>
            <cfquery attributeCollection="#queryStruct#"><cfloop condition="#running#"><cfscript>
                pos=find("?", processedSQL, startIndex);
                </cfscript><cfif pos EQ 0><cfset running=false><cfelse><cfset s=mid(processedSQL, startIndex, pos-startIndex)>#preserveSingleQuotes(s)#<cfqueryparam attributeCollection="#variables.arrParam[curArg]#"><cfscript>
                startIndex=pos+1;
                curArg++;
                </cfscript></cfif></cfloop><cfscript>
                if(paramCount GT curArg-1){ 
                    this.throwError("db.execute failed: There were more parameters then question marks in the current sql statement.  You must run db.execute() before building any additional sql statements with the same db object.  If you need to build multiple queries before running execute, you must use a copy of db, such as db2=duplicate(db);<br /><br />SQL Statement:<br />"&processedSQL); 
                }
                s=mid(processedSQL, startIndex, len(processedSQL)-(startIndex-1));
                </cfscript>#preserveSingleQuotes(s)#</cfquery>
        <cfelse>
            <cfquery attributeCollection="#queryStruct#">#preserveSingleQuotes(processedSQL)#</cfquery>
        </cfif>
        <cfcatch type="database">
            <cfscript>
            if(this.autoReset){
                structappend(this, this.config, true);
            }
            variables.arrParam=[]; // has to be created separately to ensure it is a separate object
            </cfscript>
            <cfif left(trim(processedSQL), 7) NEQ "INSERT "><cfrethrow></cfif>
            <cfscript>
            if(this.disableQueryLog EQ false){
                ArrayAppend(this.arrQueryLog, "Query ##"& ArrayLen(this.arrQueryLog)&" failed to execute for datasource, "&this.datasource&".<br />CFcatch.message: "&CFcatch.message&"<br />cfcatch.detail: "&cfcatch.detail);
            }
            </cfscript>
            <!--- return false when INSERT fails, because we assume this is a duplicate key error. --->
            <cfreturn false>
        </cfcatch>
        <cfcatch type="any"><cfscript>
            if(paramCount and curArg GT paramCount){
                this.throwError("db.execute failed: There were more question marks then parameters in the current sql statement.  You must use db.param() to specify parameters.  A literal question mark is not allowed.<br /><br />SQL Statement:<br />"&processedSQL);
            }
            </cfscript><cfrethrow></cfcatch>
    </cftry>
    <cfscript>
    if(this.autoReset){
        structappend(this, this.config, true);
    }
    variables.arrParam=[]; // has to be created separately to ensure it is a separate object
    </cfscript>
    <cfif structkeyexists(db, arguments.name)>
        <cfreturn db[arguments.name]>
    <cfelse>
        <cfreturn true>
    </cfif>
</cffunction>
\$\endgroup\$
  • \$\begingroup\$ I prefer NOT len(trim(this.dbtype)) over this.dbtype NEQ "", but looks good other than that imo \$\endgroup\$ – Matt Busche May 14 '13 at 17:00
  • \$\begingroup\$ Bruce, what I would say is this function is way too long. If you look at breaking that down into smaller private functions that will be way more readable and testable. \$\endgroup\$ – baynezy May 16 '13 at 15:16
3
\$\begingroup\$

Points:

  • I agree with @baynezy. The function is doing too much. What are your unit tests like for this they must be a nightmare? (I just noticed you said you don't have any unit tests yet. Write the tests FIRST. You are doing yourself and your clients a disservice by writing code like this without tests. It's professionally irresponsible for you to release untested code).
  • I've seen the CFC this comes out of, and you seem to be overusing/misusing the this scope. I'm fairly certain you generally should be using the variables scope for a lot of this?
  • why have you written a UDF for throwError()? CFML already has this function built in
  • when throwing an exception, you should be specifying an exception type, so calling code and then deal with said exception. Thrown exceptions are not for the human reader (ie: as indicated by your overly-long error messages), they're intended for the calling code.
  • why do you switch to <cfscript> for a simple if & throw statement? You have a lot of very small CFScript blocks for seemingly no reason. It makes the code harder to read & bloated to be switching back and forth between tags and script.
  • your k, i and s variables could probably stand having more descriptive names.
  • returning a mix of boolean or result value is a bit grim.
  • have you checked the thread safety of this method, given you're writing back to the CFC's this and variables scopes (I've seen the CFC, but have not studied it ;-)
  • returning FALSE when an INSERT fails is bad. Either bubble the error back, or raise your own one (I'd do the former).

I haven't checked your actual logic yet, but those were my initial observations from just glancing over the code.

\$\endgroup\$
  • \$\begingroup\$ throwError appends message which says the error is outside of db.execute, not inside. I try to help myself and others avoid having to read my framework code by explaining the error with how they are using it. \$\endgroup\$ – Bruce Kirkpatrick May 22 '13 at 2:41
  • \$\begingroup\$ thread safety: Only 1 query can be created and executed at a time with a single instance of this cfc. If you need to build 2 queries before executing, you'd need a duplicate of the cfc. \$\endgroup\$ – Bruce Kirkpatrick May 22 '13 at 2:44
  • \$\begingroup\$ Maybe I should use db.select, db.insert, etc instead of db.execute to make the return type consistent. Obviously you can't get a query result from an insert statement. I forgot to document the return type. "this" usage is to make those values easy to set from the outside. The example code tests the features, it's just not mxunit style yet. Thanks Adam. Hope you feel better soon. \$\endgroup\$ – Bruce Kirkpatrick May 22 '13 at 3:04
  • \$\begingroup\$ I spent many hours rewriting the project based on advice and other ideas. There are 2 cfcs now, so that one is threadsafe/cacheable (a global factory) and the other is smaller and used to build/execute queries for individual requests. The way you write queries is mostly the same aside from changing it to have more functions and variables scope instead of this scope. I also used access="package" to reduce the amount of public methods. Try running example.cfc?method=index to understand how clean the public interface is. Super time consuming to refactor and still keep it fast. \$\endgroup\$ – Bruce Kirkpatrick May 23 '13 at 3:46
  • \$\begingroup\$ I'd say you're doing something wrong if you need to worry about code-execution overhead in functions which hit external resources like DBs. Especially for the logic requirements here, which are inconsequential. I have a suspicion you try to overcomplicate things which in turn leading to you needing to try to speed things up, rather than just KISS in the first place. If I were you I'd focus on writing clean maintainable (and testable) code - none of which you are doing at the moment, based on the above - rather than whether it runs in 5ms or 4ms. \$\endgroup\$ – Adam Cameron Jun 9 '13 at 14:19
1
\$\begingroup\$

Some more feedback:

  • I'm not a fan of having variables called things like 'name' or 'cfquery', too generic and also you run the risk of using reserved words with this approach.
  • You have exactly 2 comments for nearly 100 lines of code... I prefer more comments, especially for something slightly complex
  • You define this struct, but you don't add anything to it:

    var db=structnew();
    

    then at the end you check if it has anything in it... but this IF statement is completely redundant, you'll always return true

    <cfif structkeyexists(db, arguments.name)>
                <cfreturn db[arguments.name]>
            <cfelse>
                <cfreturn true>
            </cfif>
    
  • You jump in and out of cfscript, it's messy and unnecessary. You don't really gain anything from stuff like this:

    <cfloop condition="#running#"><cfscript>
                    pos=find("?", processedSQL, startIndex);
                    </cfscript>
    
  • You don't use these variables:

    var cfquery=0;
    var k=0;
    var i=0;
    
\$\endgroup\$
  • \$\begingroup\$ cfquery is defined by <cfquery> when result attribute is not defined - it contains info about the query. I am trying to avoid all unscoped variables because these cause issues with railo debugger & performance. Only queries that return data are defined as the name attribute, so structkeyexists IS required. The db struct was so that the query name can be anything without conflicting with other local variables. The query having a provided name makes debugging easier. coldfusion doesn't support <cfqueryparam> in cfscript. new query() is CFC-based and it is inferior performance. \$\endgroup\$ – Bruce Kirkpatrick Jun 28 '13 at 18:24
  • \$\begingroup\$ If I wrote comment about every decision I make in the code, it would be so verbose and confusing if they become inaccurate. Many of my old comments are inaccurate, so I just remove them now, and focus on things like renaming variables or something. \$\endgroup\$ – Bruce Kirkpatrick Jun 28 '13 at 18:24
  • \$\begingroup\$ I understand why you define cfquery, however you then don't use it as a variable... just delete it. If however you DID want to use the query info, you just specify the result attribute, so you can use a var-scoped variable. \$\endgroup\$ – duncan Jun 28 '13 at 18:47
  • \$\begingroup\$ In CFScript you can use query.addParam instead of cfqueryparam \$\endgroup\$ – duncan Jun 28 '13 at 18:53
  • \$\begingroup\$ If old comments become inaccurate when you update the comments, update the comments, don't delete them! \$\endgroup\$ – duncan Jun 28 '13 at 18:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.