Code Reviews

Posted: July 23rd, 2012 | Author: | Filed under: Process Improvement, Software Quality | No Comments »

My friend and fellow ColdFusion developer Dave Leeds’ blog Hit The Bits! is fast becoming one of my favorite developer blogs. His recent post on Code Reviews is right on the money.


Software Quality Improvement Plan

Posted: September 7th, 2011 | Author: | Filed under: Process Improvement, Software Quality | 2 Comments »

I’m pondering an approach to cleaning up an older Fusebox-based system that I’m presently working on. The goals are to encapsulate the logic, organize the code, strengthen the code base with a both functional and unit tests. Here is my rough sketch of a plan…it’s incomplete to say the least. I’m sure that I missed a bunch of stuff, so please do me the favor of scrutinizing this in the comments :)

Step One: Build and Deployment

The Plan: First we document the existing build and deployment processes. At this point we should develop a plan for improving and automating both, integrating the appropriate functional test and unit test solution. Implement said plan. At this point functional tests should be created by the development and test teams independently of the remaining steps. Lastly, a system for performing code reviews should be implemented.

The Risks: We choose the wrong solution. This risk can be mitigated by testing the solutions suite together and analyzing the results.

The Outcome: At this point we have a formalized build process in place. We can move the code between environments easier and can run functional and unit tests as a part of the build process as they are added to the test suites. We also have cross-team cooperation between the test and development teams, and are working together to build stronger software.

Step Two: Code Together

The Plan: Create a ColdFusion style guide. This document should be limited to six pages and should offer guidance as to how to write cleaner and easier to understand code. Each developer should be a part of this document.

The Risks: The suggestions of the document will be ignored. This risk can be mitigated by ensuring that the document is concise and human-readable.

The Outcome: At this point we have a consensus among the development team as to how to style our code.

Step Three: A Testable Data Access Layer

The Plan: Now it’s time to dive into the code. Encapsulate the logic found in qry_ includes, replacing queries with calls to the appropriate AreaOfConcernDAO.getThingYouAreGetting() method call. AreaOfConcernDAO.cfc instance is created in a persistent scope only if it does not exist. Also create unit tests for each method as they are created.

Replace this…

<cfquery name="qryGetStuff" datasource="request.dsn_name">
lots of sql here that isn't indented and is kinda messy
</cfquery>

…with this…

<cfscript>
if(! StructKeyExists(persistentScope, 'AreaOfConcernDAO'){
   persistentScope.AreaOfConcernDAO = Application.SingletonManager.getSingleton('AreaOfConcernDAO');
}
qryGetStuff = persistentScope.AreaOfConcernDAO.getStuff();
</cfscript>

…and this…

<cfcomponent name="AreaOfConcernDAO">
<cffunction name="getStuff" returntype="query">
...args...
...query...
...return...
</cffunction>
</cfcomponent>

The Risks: Changes to the code will break existing system functionality. This risk can be mitigated by (1) covering the code with unit and functional tests and (2) performing code reviews.

The Outcome: At this point we have a data access layer that is fully covered by unit tests.

Step Four: Controller and View Layers

The Plan: Rewrite fusebox controller logic and views one at a time. First scope all variables in the view in the request scope. Consider using request.viewContext (it’s tidier but more verbose). Move calls to act_ and qry_ includes from the fuseaction to an include found in /controller/areaOfConcern/fuseactionName.cfm. Perform tests on each action. Scope variables every time. Formalize said tests into functional tests. Also move views to /view/areaOfConcern/fuseactionName.cfm

Each fuseaction should look like this after it’s been rewritten.

<cfcase value="subdirectory.fuseactionName">
<cfinclude template="/controller/subdirectory/fuseactionName.cfm" />
<cfinclude template="/view/subdirectory/fuseactionName.cfm" />
</cfcase>

The Risks: Changes to the code will break existing system functionality. This risk can be mitigated by (1) covering the code with unit and functional tests and (2) performing code reviews.

The Outcome: At this point we have scoped variables in the view and model and the beginnings of model-view-controller separation.

Step Five: The Service Layer

The Plan: Analyze act_ includes to determine where they are being used. Move this logic to Service objects and create unit tests for each object/method. Variables that were not scoped in step 2 should be scoped now.

The Risks: Changes to the code will break existing system functionality. This risk can be mitigated by (1) covering the code with unit and functional tests and (2) performing code reviews.

The Outcome: At this point we have model-view-controller separation with a service layer!

Step Six: Close The Gaps

The Plan: Identify gaps in unit and functional tests and close them

The Risks: none

The Outcome: At this point we are awesome. So awesome in fact that we periodically check our code to verify that we have good coverage in our test cases, and are revisiting our build and deployment processes now and then to see if we can make further improvements. Since we have amazing coverage with functional tests we can refactor the code to improve it’s speed and organization. As new requirements are formalized we can write the test cases before we write the code.