On Code Reviews

I know I said in a previous post, that enough has been said about code reviews, but I’m writing about them. I may have to do more code reviews in the near future, so it was a good time for me to gather myself and think of all the principles I should keep in mind. Also, if anyone else has any thoughts on the matter, I would love to hear them.

Define Your Goals

This seems a little weird at first. “Our goal is to write good code.” Well yes and no. The group that I manage is in charge of administering the SQL, ColdFusion and IIS boxes. My personal goal with code reviews is to ensure I don’t get paged at 2am because our servers are down. I primarily look for performance and security issues. Another member of the code review team might manage a team of programmers; they might be looking at compliance with an accepted set of conventions, because their goal is to make sure that when changes need to be made, anyone on the team can quickly make them. Still another person on the code review team might be focused on usability as their goal is to comply with government regulations.

It’s okay that there might be more than one goal. Just figure out what your organization’s goals are, and shape your team’s goals accordingly. My team’s goals are:

  1. To protect shared resources from performance problems due to inefficient code.
  2. To show off new and interesting techniques
  3. To learn from each other’s experiences

Define Standards

Now that you know what your goals are you need to come up with what the rules are. I start with the Adobe CFMX Coding Guidelines. I look at performance and security issues, and then start building out rules from there. Then I collect tidbits from other online sources, and start lumping them all together.

I need to say, before I started any of this, I read my copies of the ColdFusion MX 7 Web Application Construction Kit and Advanced ColdFusion MX 7 Application Development. These are so fundamentally important to writing good ColdFusion, that we don’t even bother telling people to read them. You start working for us, you get handed copies of them.

Publish Them

This seems to be a no brainer, but people really get annoyed when they come to a code review and are told they’ve violated our super secret standards. Now in our mind, they’re never secret. Everybody who is reading the blogs know what techniques are out of favor now right? Well no, because not everyone who codes in your organization keeps up to date with the ColdFusion blogosphere. So you need to tell them what they standards are. We do this by including a blog in our development center that has a category named “Best Practices.” We add to it as techniques or situations come up. We then force our developers to see the blog, because the rest of our development tools are on the same site.

Give Directions

We very tell people the following:

  • Make sure your application conforms to current best practices
  • Prepare CFML, HTML, CSS
    • Make a local copy of your code. Feel free to include stylesheets, but don’t bother with images.
    • Remove database, or other passwords from code.
    • Remove any unused templates
    • Clean any commented code out of your applications
    • Remove development diagnostic code like <cftimer> or <cfdump>
    • Remove database, or other passwords from code.
    • In short, code should be ready to go to production when reviewed.
  • Prepare SQL
    • All SQL going against a database server must be called in a stored procedure.
    • Use Query Analyzer to properly index tables. [Link to tutorial]
    • Use Enterprise Manager to generate scripts for all tables, including indexes. [Link to tutorial]
    • Use Enterprise Manager to generate scripts for all stored procedures. [Link to tutorial]
  • Prepare Flow Chart
    • Use Visio or some comparable program to chart the flow of your application.
    • Visio can be obtained from [Link to Central Installs location]
  • Zip up all the collected files.

You may require other things. But make sure people are told what those things are.

Read the code before the review

Nothing irritates me quite as much as a reviewer who insists on not reading the code ahead of time. To force others to wait because you say “hold on” as you flip through pages is incredibly disrespectful to you co-workers.

Some people print out all of the code, and review directly on the paper. With the size of applications these days, this is becoming more and more unwieldy. I’ve taken to using my two monitors and putting up code in one window, and Microsoft One Note in the other. That way I can just record my thoughts, or copy and paste code. Additionally, I don’t waste paper.

Make clear and concise recommendations with references to specific lines of code. Here is a sample from one of my reviews:

Game

Application.cfc

Line224

Why do you raise the logout action to an application variable? Is there a good reason for doing so?

Application_security.cfc

Line 319-326

I really like this bit of logic.

effHistroyFunctions.cfc

Line 61

This is a bit confusing: either the object you are creating either exists throughout the entire CFC (which is only one function for now, but could grow at some point.) so it should be in the variables scope and init when the cfc init’s. OR it only exists in this function in which case it should initialized with a var.

Error_exception.cfm

Line 22

CompareNoCase is prefered over eq

1 eq 1 will always be true. So this conditional will always be true so get rid of it.

Error_request.cfm

Line 23

Using the error.mailto is preferred over using a hardcoded address.

Frontier.cfc

Function frontier_calc

I feel like this function might be a little hard to maintain and troubleshoot.

Additionally it has explicit steps within it. It could be encapsulated a little further to make troubleshooting and debugging easier.

Legend

  • Must Change
  • Suggested Change
  • Stylistic Tip
  • Curiosity, Not Criticism
  • Comment

Notice also that we make allowances for several levels of severity. There are things that must be changed. But we also place an appropriate priority on less important changes so developers against a deadline know what they have to do, and what they can negotiate with us over.

The Review

The developer presents their application. They tell us what its purpose is, any specific challenges they faced, or what problem they are trying to solve. Then we usually go through the code technology by technology. (So Flex code, then ColdFusion code, then SQL, then XHTML/CSS etc.) The team gives our feedback, and then discuss any points of contention. (I think something is the best way to do it, but another review disagrees, and the developer has a specific reason for choosing to do it in a way other then what we think is “best.”)

Keep it courteous. Keep it respectful. And don’t forget to praise the good things you find.

Aftermath

The developer should make changes to their code based on the review. The reviewers aren’t done yet though. Was anything “widely accepted” as best practices new to a developer? Then it should be posted with your best practices; even if you have to repost it. You may also have to sit down with your fellow developer and help them learn a new technique.

Conclusion

Code reviews are a necessary part of running a good programming shop. No one should be exempt; even the makers of the standards. They can be painful, and ego bruising, but with a little preparation, the negatives can be downplayed, and the positives allowed to shine through.

Project: Squidhead

I’ve been reading Peter Bell and drinking the application generation Kool-Aid. To that end, I’ve been working on what I consider a development tool, but others I have talked to have called a baby framework. My gut feeling is that it might be a little bit too specialized for mass consumption, but I figured I would see if anyone else could use something like this.

For this to make sense I need to tell you a bit about my workplace environment:

  • We run ColdFusion 6 and 7, against large installations of MS SQL.
  • The Admin team (of which I am a member) mandated that developers cannot use direct SQL calls in ColdFusion, instead that we have to use Stored Procedures. I’m not going to get in the particulars of whether this is absolutely right or wrong, but it has done good things specifically for us.
  • Traditionally we have been anti-framework. A few of us have been moving towards it for the past few years, but there is still a lot of mistrust of frameworks, or patterns for that matter.
  • Traditionally we have been slow to adopt object-oriented ColdFusion. Likewise, I, and a few others came around a few years back, but others have not.
  • CFC’s took a long time to get adopted, and they are still not fully in use.

With all of that in mind, I took at look at maybe developing something that would get rid of our most repetitive tasks. So I built a stored procedure creator that I extended, and extended until now, it actually does a whole CRUD application albeit a crude one.

In a nutshell here’s what it does:

  • It inspects a Database
  • It analyzes the tables and views
  • It creates INSERT, SELECT, UPDATE, DELETE and LIST stored procedures for every TABLE.
  • It creates LIST stored procedure for every VIEW.
  • It adds them to the database.
  • It assigns the proper GRANT permissions

Then it does a second pass.

  • It inspects the database
  • It analyses all of the stored procedures not just the ones the process creates
  • It creates the <cfstoredProcedure> code for each stored procedure
  • It creates the <cffunction> to wrap the storedprocedure in.
  • It creates two sets of CFC’s: GATEWAY and DAO
  • It creates a DAO for each TABLE and VIEW.
  • It adds each <cffunction> <cfstoredproc> combo within the appropriate CFC per table.
  • It places any operation with SELECT, DELETE, UPDATE, AND INSERT in the title within the DAO CFC’S.
  • It places all other actions to the GATEWAY CFC’s.
  • It writes the CFC’s to disk.

Finally:

  • It writes Custom Tag components to produce nuggets of Editing and Listing widgets.
  • It strings all of these together to produce a simplistic CRUD application.

There are a few restrictions on its use:

  • Only works on MS SQL
  • Requires account with ability to create and drop objects
  • All tables must have identity.
  • Preferably the id should be [table name]Id (Meaning I haven’t found all the bugs)
  • Fields named “createdOn”, “updatedOn” will be automatically set
  • Field named “createdBy” will only be set on create method
  • All stored procedures should be named “usp_[tableName]_[action]”
  • Table names need to be one word, Camel case is okay
  • Action can contain dashes.

I’ve found that this sort of thing is much better received with a demonstration. Please check out this Demonstration of what it can do.

Also I have a sample of what I think its killer feature is: it analyzes all stored procedures in the database, not just the ones it wrote. So you can write your own stored procedures, press a button, and have the code generator frame out the CFC calls for it. Here is a Demonstration of this in action.

It’s worth it to say that this borrows a lot of successful patterns from Reactor for ColdFusion and ModelGlue. However, it is not as complex, flexible or powerful as either of these two. Let me make that very clear, so I don’t get any flame here. This is far inferior in terms of feature set and flexibility than either of these solutions. There is a lot this thing doesn’t do. But is also doesn’t require you to come completely over to the frameworks camp. It doesn’t require you to learn MVC. To that end, it fits my environment. I think it could be a good bridge between going it alone, and a full flexible, powerful, framework.

Would this be useful to anyone else, or am I right in thinking it’s too specific to our environment?

CFEclipse 1.3 and Vista

I desperately wanted to be one of the cool kids and try switching over to CFEclipse now that the new version is out. But Vista balked when I went to install it. It rand through the process, and then said that it couldn’t write certain files. (.bat files to be exact.)

Right clicking on Eclipse, and running it as administrator fixed this problem. I’m looking forward to give CFEclipse another try now.

Shortcut to Blogging in Word 2007

One of the features I love about Word 2007 is the fact that I can write my blog with it. It’s annoying that it takes the following steps:

  • Open Word
  • Go to big button thing in the top corner
  • Choose “New”
  • Choose “New Blog Post”

I finally figured out a way to do it, and figured I would share.

  • Search your computer for Blog.dotx.
  • On my computer it’s in C:Program FilesMicrosoft OfficeTemplates1033
  • Create a shortcut to that file.
  • Voila!

It seems so simple, I can’t believe I missed it for so long.

Running a ColdFusion Shop 2

I had more to say about running a ColdFusion shop. Hopefully I’m not overstaying my welcome on the topic.

Developers are your Users

This might be an odd bit of information, and you shouldn’t forget about the users of the applications, but developers are your users. This has a few different implications:

Development servers are production servers as far as your developers are concerned. I learned this the hard way a few years back when we lost our production environment due to a compromise. (Yeah, I had a server or two hacked, don’t judge me.) We only had a single node production environment. Part of our recovery plan was, in case of emergency, use the staging environment as the production environment. This could work for a day or two, but day three came around and the complaining started. Perhaps justified, but when you are trying to get a new production environment up, you don’t want to hear “we need to work.” In the end, you want to make sure that the development servers stay up during regular working hours.

Users have their own definition of what is and is not your responsibility. “What’s that you say? You deleted your entire directory, and want me to run a backup?” Developers will eventually either accidently wipe out files, or go down a bad path with their code and want a restore. We’ve all done it, but as an administrator you’re going to want to ensure that their problems don’t become yours. Make sure they have access to a source control product. Encourage them to use it. If you’re running on Windows enable Shadow Copy. If you do regular tape backups try and have a local backup that you can easily restore from. The idea here is that at some point you will be asked to run a restore, you want to make sure that you never really have to.

Don’t be an asshole about security. I know I just said something about being hacked, but bear with me. The first of Microsoft’s Immutable Laws of Security is:

If a bad guy can persuade you to run his program on your computer, it’s not your computer anymore.

Your developers can run code on your server; by definition you are already compromised. Don’t lock down tags. Don’t prevent object creation. A smart annoyed developer will get around you. The administrator is pretty easy to get around if you can do any sort of file manipulation. Even if this doesn’t work, there are many ways a developer can outsmart you if you are unnecessarily restrictive.

Obviously this is for groups of developers that work for the same company. I hope hosting companies don’t have this attitude. Additionally, don’t ignore external security.

Be open about your standards. A few years ago people left every code review we had saying, “Why didn’t anyone tell us about these new standards?” The reason was that the central group of administrators trolled the blogs, read articles and paid attention to what was the newest recommendations. We would talk about them, and come up with new guidelines. Problem was that we didn’t talk to the rest of the group until code reviews came up. My group started posting our standards on our Developer’s blog, and now people don’t leave code reviews feeling like they’ve been ambushed.

That’s it for now, I’m sure I remember something afterwards which can wait for an eventual part 3.

Running A ColdFusion Shop

This is a post I’ve been mulling for awhile and since my office is so cold that I can’t concentrate, I figured I would give it a shot.

In addition to writing applications in ColdFusion I’m also responsible for maintaining our environment. Our environment consists of 20 or so full time developers, with another 10 to 20 or so part time developers. Our code lives on around 20 servers, which are split up according to different versions and purposes. We run sub-environments for ColdFusion 6, ColdFusion 7 and Flex 1.5 (which is a much different animal then Flex 2.0.) The developer’s group as a whole maintains about 100 or so applications. These applications range in size from a simple one form troubleshooting reporting tool to our student portal that serves our entire student body, and won a Max Award for Education Experiences back in 2004.

I figured I would share some of my observations about what has made our environment more stable over the past three years. Maybe it can help; maybe I’m wrong and need correcting.

Develop, Stage, and Production

Break up your environment into a development area, a staging area, and a production area. We do this by having two servers per sub-environment: development/staging and production. We host development and staging on the same hardware, and use separate ColdFusion instances to separate staging from development. If you can’t make the investment in a separate server for production, then run it as a third instance.

Load Balance If You Can

Most of our production servers are actually load balanced clusters and not stand alone boxes. This means that patches that require a reboot don’t have to mean downtime for you users. You just “drainstop” to one node of the cluster. Update the deactivated node. Reboot and repeat. Being able to patch production servers during the day on Microsoft Patch Tuesday when there are Zero Day exploits? Priceless.

Write Code that Knows Where It Is

We have a couple of shared modules that allow code to determine if it is in development, staging, or production. It does this through a combination of file structure, CGI variables and registry information. Then variables can be set accordingly. That way, you don’t have to code around application state.

Don’t Produce, Publish

Don’t allow anyone, including your fellow administrators, to directly touch code in the production environment. Code should be published from staging. It doesn’t matter how. On most of our sub-environments we use a centrally available system that allows developers to push code to production. On one of our sub-environments it’s just a scheduled task that copies staging over to production every N minutes. This way developers, including yourself are forced to test in staging, before users start seeing errors due to a bad update. It also obfuscates more details about the production environment from our developers. If we add a cluster node to server, it doesn’t matter to the developers; publishing just takes care of it.

Share the Wealth

We do this three ways:

First, we write boilerplates that define a very loose framework than makes sure all applications do certain things “right.” Things like forcing login information to be posted over SSL, and encouraging the use of central code nuggets when proper.

Then we write central code nuggets, like encapsulated and logged LDAP calls, or authentication routines. We solve these problems once, and make sure everyone else can benefit.

Finally, we have our own Developer’s blog that we maintain for sharing internal information, and for posting announcements. We tied in publishing, so our developers have to look at it from time to time.

Tell Everyone How You Really Feel About Them

They’re painful, boring, cathartic, and frustrating all at the same time. Other people have written more about them ad nauseum. So I’m going to just say “No matter how painful they can be, you really need to do code reviews.” We typically make code review a condition of being allowed to publish to production.

Miscellaneous

Patch your machines. That’s all I have for now. Maybe I will have more later.

Gadgets and Widgets and Apollo, Oh My!

I’ve been building my first Vista Gadget. Dave K inspired me by writing an awesome targeted Gadget for our environment. For those of you who don’t know, Gadgets are little HTML, JavaScript enabled applications that run on the desktop and server up little bits of information like weather, or stocks, or what have you. For those of you that are saying “OSX has had them for awhile… they’re called widgets,” Shhhh, be quiet. Apple stole the idea from Konfabulator, who stole it from Apple, who probably stole it from Xerox. But let’s just say the idea has been around for awhile, and we all think it’s cool.

Anyway… I’ve been writing a Gadget, and it’s shockingly easy. However I’m annoyed at the prospect of translating it to a OSX Widget. It’s HTML, it’s JavaScript, it’s CSS; a widget is HTML, a widget is JavaScript, a widget is CSS. Why the hell do I have to do any translation? This is the age of Web Standards, and SOA, I thought I would never have to work to make a basic HTML application cross platform again. Both Microsoft and Apple deserve a little finger wagging here. You’re making us take a giant step backwards!

I could rewrite it as a Yahoo! Widget. That would make it cross platform. But I would have to rewrite it as XML and JavaScript. (Not as much fun as HTML and JavaScript. ) Plus, it’s not like Yahoo! Widgets is lighting up the Internets.

If only there was a cross platform application engine that could present applications written in HTML and JavaScript. It would harness the ease of HTML and JavaScript development with a portable runtime. It could also package them into self contained little applications that don’t need the screen real estate of an entire web browser.

Oh wait, isn’t that what Apollo is supposed to be? Can’t it do Flash and Flex too? Isn’t it also going to package whole applications, not just nuggets of applications?

I finally get it Adobe. I was lukewarm at first. But now that I see how effective these little applications can be, how easy they are to write, and my frustration with platform issues, I totally get it now. Assuming you can leverage your “Version Penetration” for Flash into seats with Apollo, you will kick the crap out of all of those other options.

But now I want Apollo. Crap.

Oh well, my plan is to keep writing the occasional Gadget, really get the whole Ajax thing down, and be better positioned to take advantage of Apollo when it is released.

New Design Done

To distract myself from peeking at Christmas gifts, I pushed ahead with a new design. You might need to refresh your browser, to see it.

From the start I decided to do it a little different. First I did a site map, figuring out how all of the content was laid out in my site. I figured out that a few sections were extraneous, so I excised them, and consolidated others.

Then I did a wireframe. I figured out that I wanted a 4 column layout. I wanted navigation to be a vertical list on the left. I wanted a side “ad” or high attention section like before. I also wanted a fixed layout. I need to brand on to my hands “You don’t design liquid layout sites that you yourself like” so I stop trying to do one.

Once all that was done I started working on a design. I started first by letting go of all of the previous colors in the site. Instead I looked for a pretty neutral palette. (I’ve been watching too many home flipping shows.) Once I had something I liked, I went back and added some color.

Finally, I decided on fonts, specifically, I picked Calibri to be the main font of the site. I know it’s a Vista font and most people don’t have it yet, so I’ve gone back and made sure it looks okay in Verdana and Helvetica. One thing I don’t like about Calibri is that it’s decidedly smaller at a given font-size than Verdana, or Arial. I’m sure someone who knows something about fonts could tell you exactly why. I am not that person.

Once the design was done, I found that I could do a few other things that I had wanted to do for a while:

  • I incorporated Google Co-op powered search. Which is awesome if you haven’t played with it.
  • I added my latest blog content to the static sub sections of my site. RSS rocks.
  • Got the download footprint of the site to be next to nothing.

I have few other things left to do, like prune away all the old sections of the site, but for the most part I’m done.

Contemplating New Design

Not that original a subject, but I figured I might at least talk about it, and solicit some advice.

Due to a number of reasons I’m looking to update the look of Aarrgghh!.

  1. It’s not looking good cross-browser these days.
  2. Dave Konopka just did a redesign that looks awesome.
  3. I’m spending a fair amount of time dealing with a redesign of a site here at work.
  4. I do this about every 6 months or so anyway, to keep my skills sharp.
  5. I might add a little Ajax-y stuff to practice it.

My process until now has been very haphazard. I basically go to CSSZenGarden, WebdesignFromScratch, and OpenSourceWebDesign, and browse. I shop for little things I like from various examples there, and try and tie the whole thing into one design.

This tends to leave me with a site that mostly works, but doesn’t really feel right, but I put up with it until I decide to do it all over again.

So instead, I’m going to take a longer look at all this. Then, I’ll actually try my hand at wireframing before even look at design.

Anybody got any advice out there?