Advanced ColdFusion Contest Entry 1: CodeCop
Welcome to the first entry review for my Advanced ColdFusion contest. Our first entry is from Steve Bryant. I have to say that if every other entry is like this, I will be incredibly happy. You may download it using the Download link below.
As a reminder, I will be picking apart these applications and pointing out things I think are done well, and done badly. This is not meant to attack the author, but merely to give my opinion on how he did things. I am not perfect myself, but one of the points of this exercise is for us to learn from each other. So, enough touchy-feely crap, let's get to it.
Presenting CodeCop
I'm going to begin by commenting on something that isn't ColdFusion related, or even contest related. I currently run a few open source projects, and I think I'm getting the hang of how to do it right. One nit picky thing I saw right out with his zip file is that the install files are intermingled with the rest of his code. I always recommend folks use a "install" subfolder. This makes it easier to find the installation instructions, and makes it easier to remove the install files from a production box. Since install files could potentially be dangerous, it is always a good idea to make it easy to remove them.
My next issue was with the installation instructions. In particular, he mentions that you will be asked to pick a datasource, but he doesn't say why. The application installs itself into a database, but nowhere is that made clear. I was naturally concerned so I first picked some old database. The application did not like that (but handled the error gracefully). I then made a new DSN with an empty database and that made things work nicely. Note to Steve - you simply cannot provide enough information in your instructions. Always provide as much as possible. For example, would any DSN have worked? SQL Server worked, but what about MySQL? As a quick side note - he also provides a "contest_notes.txt" file which talks about his thought process. To be honest, that's pretty cool. None of my applications talk about why I did stuff, the philosophy behind design decisions, etc.
So - after a bit of fuss setting things up, I had the application running. The first thing you are asked to do is to pick a folder of code to scan. You have the option to specify particular extensions or to simply use all extensions. (I assume he meant cfc and cfm and not really *.*. If he did, he should document that.) You are also prompted to select a rules package. I chose the default. After a few seconds your report shows up.
The report gives you a nice breakdown of all the issues discovered in the code you processed. You can then jump into the file and see the issue, or click on the issue type and get more information about what it means. Reports are automatically stored into the database and you have the option to export to PDF as well. All in all, the UI is very nice and clean. Now let's talk about the engine.
When I created this contest, it was my assumption that folks would extend the rules engine by writing code, much like how I built Canvas. Steve however made the configuration all web based. Rules are added and edited via the interface. Rules have various metadata associated with them so you can assign a severity and other properties. Rules can be either regex or tag based, but here again the lack of documentation makes it a bit difficult to know which to use. For example, for a tag based rule, do I do <cffoo> or just cffoo? Looking at another rule, it seems as if you write it without the brackets. However, my test threw an error when I tried to search for cfquery. You can also write custom code, again via the interface, to do specific checking for the rule. I'd imagine this is a bit hard to test though.
Rules can be placed in packages, which is nice, since you may not like the "Prefer structKeyExists over isDefined" rule. One thing that would have been nice, but potentially complex, is the ability to assign different rule priorities in packages. So for example, I may want to keep the structKeyExists rule, but change the priority in a different package.
Another cool aspect of this application is that it doesn't have to run under the ColdFusion Administrator, and if it doesn't, it actually changes the skin used to display the application.
So, that's it. If you want a sample of the output, check this PDF, it is a report created by running the default rule package on CodeCop itself. All in all I'm very impressed with this product, and by how the extensibility doesn't require you to write code. Again, that isn't what I expected, but I like being surprised. The primary issue he has now is simply documentation. Don't forget that you can download this application below.
Comments
I like how you can click right into highlited source code to view mistakes, and I really like how when you click on an error, it shows a list of all templates that use that error. I have not made any rules yet, but I really like this so far!
I ran this against one of my projects, and one of the things it noticed right away is my use of the 'evaluate()' function. I know thats generally a no-no, but how else would you get the value for a variably named variable? ie Evaluate("URL.level#Numberformat(i,"0000")#")
URL["level#Numberformat(i,'0000')#"]
FYI - Steve has a new zip. I have replaced the zip as of RIGHT NOW. If you downloaded, redownload. It fixes the bug I mentioned with a tag rule.
Oh duh, because the URL is actually a structure of values, I gotcha.
Thanks man!
Thanks for all the feedback! I will try to get my documentation updated soon.
What you see for the DSN choice actually varies based on where the installation is. If CodeCop can find a list of datasources (if it is running on CFMX 6.1 or in the administrator of CFMX 7), then it will return a list of datasources for the supported database types.
If you add a new DataMgr.cfc for a new database type and refresh the code (by setting URL.reinit to 1), then the new database type will be supported (so you could add DataMgr_Ingres.cfc and the whole application would support Ingres).
Nonetheless, I should have documented that and plan to do so.
Justice,
Sorry for the brief answer. Basically all ColdFusion variable scopes are structure and can be accessed with structure notation as given. So, URl.myvar is the same as URL["myvar"].
Ray, I think this particular contest idea was really cool - not only as a learning exercise, but also because the resulting applications are really useful.
As an FYI, next Friday I'll have the second entry.
Error Executing Database Query.
[Macromedia][SequeLink JDBC Driver][ODBC Socket][Microsoft][ODBC Microsoft Access Driver] Syntax error (missing operator) in query expression 'chkRules.SeverityID = chkSeverities.SeverityID INNER JOIN chkCategories ON chkRules.CategoryID = chkCategories.CategoryID'.
The error occurred in D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\Rules.cfc: line 101
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\Rules.cfc: line 242
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\Transfer.cfc: line 69
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\CodeCop.cfc: line 68
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\Application.cfm: line 73
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\Rules.cfc: line 101
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\Rules.cfc: line 242
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\Transfer.cfc: line 69
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\sys\CodeCop.cfc: line 68
Called from D:\inetpub\wwwroot\CFIDE\administrator\CodeCop\Application.cfm: line 73
99 : INNER JOIN chkCategories
100 : ON chkRules.CategoryID = chkCategories.CategoryID
101 : WHERE RuleID = <cfqueryparam value="#arguments.RuleID#" cfsqltype="CF_SQL_INTEGER">
102 : </cfquery>
103 :
--------------------------------------------------------------------------------
SQL SELECT RuleID,RuleName,RuleType,TagName,Regex,Description,allExtensions,UUID,CustomCode, chkSeverities.SeverityID,SeverityName,rank, chkCategories.CategoryID,CategoryName, '' AS Packages, '' AS Extensions, '' AS ExtensionNames FROM chkRules INNER JOIN chkSeverities ON chkRules.SeverityID = chkSeverities.SeverityID INNER JOIN chkCategories ON chkRules.CategoryID = chkCategories.CategoryID WHERE RuleID = (param 1)
DATASOURCE CodeCop
VENDORERRORCODE -3100
SQLSTATE 42000
Read through:
http://ray.camdenfamily.com/index.cfm/2006/2/8/Ask...
and
http://corfield.org/blog/index.cfm/do/blog.entry/e...
for your answer. Surprised me too when Ray enlightened me.
Good question about the compiled list of rules. I just made a blog entry to try to compile a list. Head on over there and make your suggestions!
http://steve.coldfusionjournal.com/coldfusion_codi...
Rich,
I know I tested this on Access at some point in development, but I must have made some changes since then. I don't see anything in that SQL that stands out, but maybe I accidentally used a reserved word or something. I will check it out.
Thanks, that helps a lot! :-) (It's also disapointing, because I really like the simplicity and readability of isDefined().)
Which means that in order to support Access, I have to change all of my pretty inner joins that are cluttered up by nested parenthesis or change to a less clear comma delimited list of tables in the from clause and do the joins in the where clause.
I'm not happy with Access at the moment.
RE: The Evaluate() comment in the screenshot.
People always claim Evaluate is slower, and never offer any evidence to back it up.
I've done simple tests myself, comparing thousands of iterations of Evaluate("Form.#Var#") against Form[Var], and there was no notable difference between them.
Obviously Form[Var] is far more elegant, but I get irritated by people making speed claims without checking/offering proof - especially if the advice is being given from an authoritive source, such as a code checking application.
</rant>
Check this for a view from a core CF engineer:
http://blogs.sanmathi.org/ashwin/2006/07/24/whento...
"As has been noted often enough, evaluate() and iif() are not the most performant of functions, since they do need to dynamically compile the expressions that they’re being asked to process."
Thanks for that link though, it's a nice explanation, and has a good clear summary on when/not to use them...
> In summary - use evaluate() and iif() in situations
> where the expression to be evaluated remains static over
> time. Using them for dynamic expressions will prove
> inefficient, and may well negate any benefits that
> static expressions enjoy.
Steve, thank you for sharing.
Ray, a big thank you to you too.
cannot convert the value "" to a boolean
The error occurred in D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sys\Reviews.cfc: line 208
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sys\CodeCop.cfc: line 120
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sebtags\sebForm.cfm: line 779
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sebtags\sebForm.cfm: line 756
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sebtags\sebForm.cfm: line 755
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sebtags\sebForm.cfm: line 753
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sebtags\sebForm.cfm: line 326
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sebtags\sebForm.cfm: line 233
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\sebtags\sebForm.cfm: line 233
Called from D:\CFusionMX7\wwwroot\CFIDE\administrator\CodeCop\index.cfm: line 34
206 : <cfloop query="qRules">
207 : <!--- If this is a file that should be checked for this rule --->
208 : <cfif allExtensions OR ListFindNoCase(ExtensionNames,ListLast(ListLast(FileInfo.FullName,"/"),"."))>
209 : <!--- Run check for this rule type --->
210 : <cfset variables.RuleTypes[RuleType].checkRule(ReviewID,RuleID,FileInfo)>
Anyone else had the same problem? Not sure if it's my dodgy cfc's I'm testing or if it's a problem with CodeCop...
It looks like a problem with CodeCop. I think I see what caused it.
Would you mind dropping me a line with your information so I can test it in a situation closely matching your?
Either on my web site or send me an email (my first name at my domain name).
http://www.bryantwebconsulting.com/contact.cfm
If anyone else runs into trouble, let me know as well. I am hoping to have the first public beta out soon (feel free to use this copy until then).
You may want to ping Steve Nelson (of Fusebox fame). I remember way back in the day, he had a code checking tool as well. Many of us (myself included) sent him ideas for rules at the time. Perhaps he'd be willing to share...
That is a good idea. I will do that.
Tom,
I just uploaded a new build to my site that should fix issues with Access. The bad news is that you will have to use a different database or delete the "chk" tables from the one you are using in order for the fixes to be effective (CodeCop will recreate them correctly).
http://www.bryantwebconsulting.com/products/codeco...
I am also hoping to create some real documentation as soon as I get time.
Good idea!
Actually, that is there already (even in my original entry).
The packages page has an "Import Package" link that will allow you to import a package from an XML file.
If you edit a package, that page as an "Export Package" link which will export the XML for that package.
This will allow you to share packages and rules with other developers.
I also just put out a new build which fixes the problems with MS Access, improved the installation instructions, and adds CFC Introspection.
http://steve.coldfusionjournal.com/codecop_beta.ht...
Thanks for a very cool product, but I shudder when I run it against code I ran 4 years ago...
Great, your update works just fine. A great little application. I just hope it doesn't start handing out fines now. Have enough trouble on the roads.
Tom
Another issue I have with this application is - not enough documentation. What does this do, how can it help me do my job better, convince me why I should use it.
I know I need documentation for it. I just have to find the time to do that.
I just installed the latest version from your site and it's working pretty well. I did notice an error, however, when I click on By Categories, By Files, or By Rules links in the report.
<code>
The column name (FileName_UCase) that you specified already exists in this query.
Column names must be unique.
The error occurred in C:\CFusionMX7\wwwroot\tools\CodeCop\sebtags\sebTable.cfm: line 360
Called from C:\CFusionMX7\wwwroot\tools\CodeCop\sebtags\sebTable.cfm: line 352
Called from C:\CFusionMX7\wwwroot\tools\CodeCop\sebtags\sebTable.cfm: line 341
Called from C:\CFusionMX7\wwwroot\tools\CodeCop\sebtags\sebTable.cfm: line 161
Called from C:\CFusionMX7\wwwroot\tools\CodeCop\sebtags\sebTable.cfm: line 161
Called from C:\CFusionMX7\wwwroot\tools\CodeCop\sebtags\sebTable.cfm: line 1
Called from C:\CFusionMX7\wwwroot\tools\CodeCop\review-rules.cfm: line 42
358 : <cfset ArrayAppend(aSortVals,UCase(qTableData[ThisTag.qColumns[i].sortfield][CurrentRow]))>
359 : </cfloop>
360 : <cfset QueryAddColumn(qTableData, "#ThisTag.qColumns[i].sortfield#_UCase", aSortVals)>
361 : </cfif>
362 : </cfloop>
</code>
Thanks for the catch.
It looks like the newer version that I had available as an example implementation of DataMgr 2.0 Beta didn't have the error, so I made that the primary download for CodeCop.
As an aside, I haven't forgotten about this project. My spare time has been limited and mostly dedicated to DataMgr 2 lately. Hopefully I will get the install cleaned up and release CodeCop 1.0 in the somewhat near future.
Thanks,
Steve
Error line: CodeCop\com\sebtools\DataMgr.cfc: line 1522
line 1522: <cfquery name="qQuery" datasource="#variables.datasource#"><cfloop index="i" from="1" to="#ArrayLen(aSQL)#" step="1"><cfif IsSimpleValue(aSQL[i])><cfset temp = aSQL[i]>#Trim(PreserveSingleQuotes(temp))#<cfelseif IsStruct(aSQL[i])><cfset aSQL[i] = queryparam(argumentCollection=aSQL[i])><cfswitch expression="#aSQL[i].cfsqltype#"><cfcase value="CF_SQL_BIT"><cfif aSQL[i].value>1<cfelse>0</cfif></cfcase><cfcase value="CF_SQL_DATE">#CreateODBCDateTime(aSQL[i].value)#</cfcase><cfdefaultcase><cfif ListFindNoCase(variables.dectypes,aSQL[i].cfsqltype)>#Val(aSQL[i].value)#<cfelse><cfqueryparam value="#aSQL[i].value#" cfsqltype="#aSQL[i].cfsqltype#" maxlength="#aSQL[i].maxlength#" scale="#aSQL[i].scale#" null="#aSQL[i].null#" list="#aSQL[i].list#" separator="#aSQL[i].separator#"></cfif></cfdefaultcase></cfswitch></cfif> </cfloop></cfquery>
Ray's year old blog entry probably isn't the best place for support.
The best thing to do is go to the RiaForge page (listed below) and make sure you are running the latest version.
If you still have the bug, you can report it there or go to my site and fill out the contact form and I will be happy to see what I can do to help you out.
http://codecop.riaforge.org/
Nice one, slick an clean.