Saturday, June 1, 2013

Introducing CodeChecker - a code review automation tool

Code review is an important part of any software development cycle.  It helps ensure to some degree the quality, coherency, and consistency of an application to the team's objectives.

When conducting code reviews, I much rather like to focus on the logic of the code and that it aligns to the direction our team is going rather than scanning line upon line for code that preferably should not be used such as output="true" in a cffunction or <cfif myVar IS 1>.

That is why I'd like to introduce CodeChecker! It is a code review automation tool that helps take away the tedious (and boring) aspects of reviewing code.  It has a UI to be ran from the browser, or you can call the CFCs directly in MXUnit.  All of the rules are defined as regular expressions, and it also integrates the popular QueryParamScanner and VarScoper components. My regex skills admittedly are not great, so anyone out there who would be willing to optimize them please contribute to the project on GitHub.  Below are some of the highlights.  More documentation can be found on the project's wiki.
  • Rule Categories
    • Security
    • Performance
    • Standards
    • Mainenance
  • Rules
    • Prohibit client scoped variables in a CFM page
    • File upload warnings to ensure they use the accept attribute and check for valid file extension and MIME type.
    • Prohibit nested cflock tags
    • Prohibit ParameterExists()
    • Prohibit IsDefined()
    • Prohibit Evaluate()
    • Prohibit DE()
    • Prohibit IIF()
    • Prohibit StructFind()
    • Prohibit DecrementValue()
    • Prohibit IncrementValue()
    • Use Len() instead of is "", is not "", etc.
    • Prohibit SetVariable()
    • Prohibit cfquery in a CFM page
    • Prohibit shared scope variables (form, application, url, session, cgi, client, request, cookie) in a CFC
    • Prohibit IS and GT for boolean tests
    • Prohibit IS/IS NOT when comparing numbers
    • Prohibit EQ/NEQ when comparing strings
    • Prohibit mathematical operations on strings
    • Prohibit the ampersand concatenator on numbers
    • Prohibit empty cfcatch blocks
    • Prohibit output=true in cfcomponent and cffunction
    • Require init method in CFCs
    • Require onMissingMethod method in CFCs
    • Require hints in cfcomponent, cffunction, and cfargument
    • Use ArrayNew() instead of ArrayNew(1)
    • Prohibit arguments-scoped datasource (since the datasource should be set on object instantiation)
    • Prohibit cfabort and abort()
    • Prohibit cfdump and writedump()
    • Prohibit cflog and writelog()
    • Prohibit console.log()
  • Third Party Plugins
Every team/developer has different needs and preferences, so feel free to remove any of the rules that you don't require.  As I mentioned above, feel free to contribute new rules and optimize the ones I've defined.  Also, any comments are welcome!

Special thanks/credit to Steve Bryant's CodeCop project for inspiring this.

9 comments:

  1. This looks really impressive. You have several features (notably integration with QueryParamScanner and VarScoper) that I have been intending to add to CodeCop for quite some time but unable to find the time to work on it.

    I think I might just try out CodeChecker instead.

    ReplyDelete
  2. Thanks! I'll definitely be checking this out.

    Is this project able to be extended with community information derived from the Adobe BugBase? For example, I reported a serious (yet currently "unverified") ColdFusion 10 bug regarding the use of isNull(). It'd be great to be able to subscribe to a list that identifies reported/unverified/known bugs by CF version and adds these rules on-the-fly.

    By the way, here's the isNull() bug with verifiable sample code:
    https://bugbase.adobe.com/index.cfm?event=bug&id=3556076

    ReplyDelete
  3. Nice! Rule.cfc line 195 should wrap the pattern in single quotes, else you get false positives.

    ReplyDelete
  4. also, this is actually incorrect:

    Use ArrayNew() instead of ArrayNew(1)

    It should be the other way around. Unless you mean to say use [] instead of ArrayNew(1)?

    ReplyDelete
  5. @YouthPastor - I like that idea, but I'm not sure where the hook would be to glean the rule. I'm looking at your bug report and nothing's jumping out at me. Thoughts? As an aside, I have a couple TODO comments in the Rules.cfc to prohibit reserved words and deprecated functions. It would be nice to pull those in as well automatically.

    @Andy - nice catches. yes, I meant []. I just used the 042 octal value instead. I also added 047 octal value for single quote expressions.

    ReplyDelete
  6. @Unknown - thanks for the kind words. Are you Steve?

    ReplyDelete
  7. Is there a way to easily filter which category of rules are used during the scan? I'd like to add some new custom rules, but have them only used under certain circumstances.

    To add this to the form, identify all unique categories from the Rules struct and generate checkboxes to toggle on/off. (No selected checkboxes = run all tests)

    ReplyDelete
  8. Looks very promising. At the moment I'm getting timeouts when checking a directory, but maybe that's not how it's meant to work?

    ReplyDelete
  9. @YouthPastor - there currently is not a way to turn rules off on a scan by scan basis. I'll look into adding that.

    @Joost - it certainly is meant to scan directories as well as individual files.

    How many files are in the directory/directories you are scanning?

    The project I've been testing this on has over 1,300 files in it and it scans in 16 seconds including the output of the results.

    Is the timeout in the output of the results or the actual scan? Try commenting out the results output to narrow down the issue.

    Also, what is your request timeout set at?

    Let me know if that helps.

    Feel free to ping me on GTalk or Twitter.

    ReplyDelete