Showing posts with label Code Reviews. Show all posts
Showing posts with label Code Reviews. Show all posts

Sunday, August 19, 2007

Heroes, Villains, and Software

I'm a big fan of comics, and I'm also a big fan of software development. Coincidentally, I see metaphors and analogies for software development just about everywhere I look.

Comic books have come a long way from the days when it was just about the guy wearing the mask or the brightly colored costume who rushed in to save the day. In the early days, the stories were pretty straightforward: the bad guys were always easily identified, and so were the good guys. But stories these days have become more complex, more adult, and they tackle tough issues. But one common element persists: the heroes and villains tend to wear costumes or masks that exemplify their heroic or villainous persona.

The attire of a hero or villain isn't simply done for theatrical reasons (although there is certainly a large and undeniable element of theatricaility to it). The mask grants the wearer anonymity; those who behold the wearer have no idea who the wearer is, and with that anonymity the wearer is emboldened to do things he or she might never do or have the courage to do if people knew who he or she was.

Anonymity grants the wielder power. What separates the heroes from the villains is a sense of responsibility and what they choose to do with it. A villain might have a sense of responsibility but choose to ignore it, and use his power for personal gain.

When a civilian in a comic book views a mask-clad individual for the first time, they don't really know whether that individual is a hero or a villain, or just some bloke in a costume. They might be able to infer some things from the costume, but they have to trust their instincts. A great failing of classic villains seemed to be that they went out of their way to look villainous; they were so easily identified that they were easy to avoid or locate and defeat.

But the greatest villains actually looked like one of the good guys, or just a regular Joe. They managed to garner the trust of those around them, and then used that trust against those who placed it in them. You see, sometimes, wearing no costume at all is a mask in and of itself.

So now we come to the part where we apply this to software.

All software wears a mask. It's called the user interface. It doesn't matter if it's a desktop application, a Web application, a framework, a Web service, a device driver, or anything else. The mask is the face that your software presents to the public, how the public perceives the software when they first look at it.

Most software presents itself as the next silver bullet to any given problem domain. After all, what software doesn't want to be perceived as a Hero? A user, looking at your software for the first time, is forced to place a certain amount of trust in it because all they can see is its mask. And the mask grants the software anonymity and power.

Unless you're giving the user the source code, and the user knows how to read it, and can build it themelves, they're at your mercy. They're trusting you to be heroic on their machine: to have a sense of responsibility, and to use the power you've been granted wisely.

Sadly, much precompiled software relies on its anonymity to conceal its true state: that it is miserable, ugly, unmaintainable, untestable, bloated, unreliable, and unsafe. Software of this nature is hardly heroic; rather, it's villainous. It might handle user data with reckless disregard for integrity, security, or consistency. It might crash frequently, and for no apparent reason. It might install spyware or adware. It might log keystrokes. It might be riddled with bugs, be poorly documented, and have no consistent coding model or standard. It might use an inconsistent user interface paradigm, or hog system resources. It might break other software. It might do all of these things.

The question we are compelled to ask is this: What is hiding behind the mask of the software that we're writing, right now, at this moment? Is it heroic? Or is it villainous? Despite claims by certain advertisers, it isn't really enough that "it just works." It has to work well, and responsibly.

Some of the greatest heroes in comic book lore were brave enough to take their masks off so that people could see them and know who they were. Would you be willing to open your code base up to let the world see what you're doing? Would you feel comfortable letting everyone see the code that you write and what it's doing? Would you be confident or proud of your work? Or would you feel shame? Embarrassment? Even guilt?

Not all software can be heroic. There are always mitigating circumstances. But we can all strive to be heroic. It's when we strive to be villainous that we need to be worried. Or when we're writing villainous software, and we know it, and we choose not to do anything about it. We all have a choice. We can choose to make great software that behaves responsibly, and does what it says its going to do, or we can abuse the trust placed in us and serve ourselves.

Take stock of the software you're writing. Find out what's behind its mask. If it's not software you'd be prepared to unmask in front of its users (aside from intellectual property issues), chances are, something somewhere has gone horribly awry. What are you going to do about it?

Wednesday, July 11, 2007

A Parameter Validation Framework

I need some help here. I'm hoping you'll review the following "framework" and give me your feedback. I need to know (1) if it's a good idea, (2) if the architecture is sound, and (3) how it can be improved. If it's a generally useful idea, I'll open it up on an open source project forum where it can be expanded for general use. Otherwise, I'll go back to the drawing board. (I'm not sure at this point if it's a worthwhile idea.)

The Background

In reviewing my projects, I noticed that I tend to be lazy about validating parameters to methods. I reviewed a number of projects that I was working on and noticed that it tended to be true no matter what the project was, or what the parameter types were. When I sat back and thought about it, and paid attention to myself doing it, it came down to two primary issues that led to me avoiding it:

  • I was usually in a rush to get a product out the door. This tends to always be the case, since the product's schedule is out of my control, and there isn't much that I can do about it. I can only negotiate for a minor change in the project's time. Then, I'm left to work with the amount of time I'm given. So I try to do the best work I can in the time I'm allotted, and hope to high heaven that it's good work. That often means that I code like hell (classic software mistake), and hope that nothing breaks.
  • Parameter validation code tended to be verbose. When I really wanted to say, "Parameter y should be a positive number," the resulting code looked like this:
    If myParameter <= 0 Then
       Throw New ArgumentOutOfRangeException("myParameter", "Must be a positive number.")
    End If

These two issues alone might not seem like a big deal in and of themselves. But when it comes to encouraging code quality, I take them very seriously. I abhor sloppy code, especially when it's mine. I want to be sure that parameters are valid, and that when a method receives parameters, they are checked rigorously to ensure that I'm getting valid values throughout any system I'm developing.

The Goal

I set out to come up with a way to make it easier to validate parameters. I wanted a way to make it so easy to validate them that I would find it enjoyable to do so. I wanted it to make my code more legible, with a minimal amount of impact on its performance. Knowing that I basically lack discipline, I knew I needed a software solution that would exploit Visual Studio's capabilities to remind me to check for tests that I might not have normally thought of. (We have a tendency to inadequately document the requirements—again, due to time constraints—and I need to be able to think about logical tests that make sense for the parameter as I'm coding them.)

So the goals for the framework are:

  • Ease of use. This was the number one design goal. If the thing wasn't easy to use, it was pointless to build it in the first place, since it was being designed to encourage validation of parameters. If the framework made it harder to do that, it was defeating the purpose.
  • Understandability. The code should enhance the readability of the code.
  • Performance. The methods were going to be called with a high degree of frequency. It was critical that the methods have a very low overhead on the call stack. If at all possible, the number of objects being created should be kept to a minimum. However, because we're adding code, some overhead is unavoidable.
  • Reliability. It has to work. And it has to work reliably and predictably every time.
  • Extensibility. It has to support other data types.
  • Maintainability. The framework's code has to be easy to read, understand, and maintain. We do not want to have to bloat the development time of the projects that rely upon it with weeks of maintenance time just to fix and expand the parameter validation library.

The Solution

I set out to come up with a way to make it easier to validate parameters. I wanted a way to make it so easy to validate them that I would find it enjoyable to do so. I wanted it to make my code more legible, with a minimal amount of impact on its performance. I wrote a set of classes that, I believe, accomplished that. The resulting "framework" (if you can call it that) is based on the Assert.That model popularized by NUnit and JUnit. When you want to prove that a parameter is positive, you write the following:

Sub Foo(ByVal integerParameter As Integer)
   Validate.That(integerParameter, "integerParameter").IsPositive()
   ' Do something interesting with integerParameter
End Sub

It's short, sweet, and to the point. If integerParameter contains any value that is less than 1, an ArgumentOutOfRangeException is thrown, with an appropriately formatted message bearing the parameter's name.

The solution involves the following classes:

Class Description
Validate A class factory that instantiates strongly-typed parameter validator objects. These are derived from ParameterValidatorBase. Provides the heavily overloaded That method.
BooleanValidator Inherits from ParameterValidatorBase. Provides methods for validating Boolean parameters, such as IsTrue and IsFalse.
ConnectionValidator Inherits from ParameterValidatorBase. Provides methods for validating IDbConnection parmaeters, such as IsNotNull, IsOpen, and IsClosed.
DateValidator Inherits from ParameterValidatorBase. Provides methods for validating date parameters, such as Equals, IsBetween, IsGreaterThan, IsGreaterThanOrEqualTo, IsLessThan and IsLessThanOrEqualTo
EnumValidator Inherits from ParameterValidatorBase. Provides methods for validating enum parameters. This is the only parameter validator that relies on reflection. It's sole method is IsValid.
IntegerArrayValidator Inherits from ParameterValidatorBase. Provides methods for validating integer array parameters, such as IsNotNull and IsNotEmpty.
IntegerValidator Inherits from ParameterValidatorBase. Provides methods for validating integer parameters, such as Equals, IsGreaterThan, IsGreaterThanOrEqualTo, IsInRange, IsLessThan, IsLessThanOrEqualTo, IsNegative, IsNotNegative, IsOneOf, IsPositive, and NotEqualTo.
ListValidator Inherits from ParameterValidatorBase. Provides methods for validating IList parameters, including IsEmpty, IsNotEmpty, IsNotNull, and IsNotNullOrEmpty.
ObjectValidator Inherits from ParameterValidatorBase. Provides methods for validating any Object parameter, including IsNotNull and Equals.
ParameterValidatorBase The base class for all validators. Provides the name of the parameter and a protected property to store the parameter's value.
StringArrayValidator Inherits from ParameterValidatorBase. Provides methods for validating string arrays, including IsNotNull, IsNotEmpty, and IsEmpty.
StringValidator Inherits from ParameterValidatorBase. Provides methods for validating strings, including Contains (overloaded), Endswith (overloaded), IsNotEmpty, IsNotNull, IsNotNullOrEmpty, and StartsWith (overloaded).
TransactionValidator Inherits from ParameterValidatorBase. Provides methods for validating IDbTransaction parameters, including HasValidConnection, HasOpenConnection and IsNotNull.

Other parameter validator types are being added as the need for them arises.

When a call is made to Validate.That(), the framework instantiates an appropriately typed parameter validator object. The caller then invokes one or more of the methods on that object to prove that the parameter is valid. The parameter validator merely tests the condition specified by the method name; if the test evaluates to False, an appropriately typed exception (derived from ApplicationException) is thrown. The parameter validator is responsible for properly formatting the message and passing the name of the parameter; this is spiffy because some of the exception objects are inconsistent about the order in which the name and message parameters are passed to them. The framework standardizes the order through the use of overloads: parameter, name, message.

For example, the source code for the ConnectionValidator class looks like this:

Option Explicit On 
Option Strict On

Imports
System.Data

Namespace Validation

Public Class ConnectionValidator
Inherits ParameterValidatorBase

Friend Sub New(ByVal connection As IDbConnection, ByVal name As String)
MyBase.New(connection, name)
End Sub

Public Sub IsClosed()
If Not Connection Is Nothing Then
If (Connection.State And ConnectionState.Closed) = 0 Then
Throw New ArgumentException("Operation requires a closed connection.", Name)
End If
End If
End Sub

Public Sub IsNotNull()
If Connection Is Nothing Then
Throw New ArgumentNullException(Name)
End If
End Sub

Public Sub IsOpen()
If Not Connection Is Nothing Then
If (Connection.State And ConnectionState.Open) = 0 Then
Throw New ArgumentException("Operation requires an open connection.", Name)
End If
End If
End Sub

Public Sub IsValid()
IsNotNull()
IsOpen()
End Sub

Private ReadOnly Property Connection() As IDbConnection
Get
Return DirectCast(InnerValue, IDbConnection)
End Get
End Property
End Class

End
Namespace

The Pros & Cons


I've noticed that I am, indeed, far more likely to validate parameters now with this framework. I'm catching a lot more defects with it as well. The idea behind it seems to be working. However, it does have a few problems:



  • It's failing to meet its extensibility requirement. In order to add new types, I have to hand-code a new overload to the That method into the Validate class. I need to find a new way to do that.
  • It's not properly localized. (You can see the hard-coded English strings in the code sample above.)
  • It adds overhead to the stack trace, which can be confusing to users who don't know what it is.
  • It lacks a way to conveniently perform multiple tests on the same parameter validator object (aside from Visual Basic's With...End With block, which just looks unnatural). The current implementation tends to ask you to create multiple objects to work with the same parameter; although the objects maintain very little state (two variables, both object references) it's still more than I'm comfortable with.

Despite its drawbacks, most of which are addressable, its benefits appear to be worthwhile. My code quality is rapidly improving. Defect rates are dropping noticeably (and I feel comfortable attributing a good portion of it to better parameter validation).


The Request for Comments


So there you have it. At this point, I would really like to hear back from the community, and find out what you think of this thing. Should I be doing this? Should I be doing it better? How would you improve on it? What would your concerns be if you were using it or designing it yourself?


Remember, I'm the quintessential vacuum coder here, so your input is valuable to me.

Thursday, July 5, 2007

Vacuum Coding Syndrome: The Need for Peer Review

I stumbled across this interesting quote today as I was looking for information about the effectiveness of peer code reviews:

There are some thing's you can't unit test. You can't unit test design or architecture. And sometimes you can get unit tests wrong—you might assert the wrong things. There is always a place for human eye-balling of code.

Matt Quail, at JavaOne 2007

This statement resonates profoundly with me because of the situation that I'm in. As I've mentioned previously, I code alone. I have no peers with whom I work and can share designs and submit them for review. So when I make a bad design decision and then implement it, I'm stuck with it. Lately, I've been going over my code, and when I come across some of my older stuff, I'm frequently left wondering just what in the hell I was thinking.

There are designs and architecture decisions that I made three years ago that seemed perfectly reasonable at the time, and now I look at them with abject horror.

In one case, I knew that our database architecture was going to be massive, and that it was going to be changing frequently. I also knew that the data model classes were going to have to be constantly updated to stay in synch with the database, and that there was no way I was going to be able to do all that manual work without missing something. I needed to be able to update the database schema, push a button, have the classes regenerated, and then move along to focus on the business logic.

So I wrote a program that did that. It was, to be sure, a very fast, very efficient program. But it didn't generate very pretty code. In fact, it generated a lot of classes that we never use. For every table, it generated an insert, update, delete, exists, delete all, and select all stored procedure. For each stored procedure it had generated, it created a wrapper class that correctly created the SqlCommand, populated its parameters, and invoked the procedure. It also generated a data model class and a collection class. It also generated a strongly typed primary key class, and the DAC class that invoked the stored procedure classes.

Now that's a lot of code. At the time, it seemed like an act of sheer brilliance, and I marveled at the fact that if the schema changed I could simply regenerate the classes and know that they matched the schema. But the number of stored procedures quickly grew out of proportion to what we actually needed for the system we were building, and the vast majority of them were never used. And if those procedures were never being used, you can bet that the generated classes were never being used, and neither were the methods on the DAC classes.

That program, thankfully, was decommissioned early on, but it left a massive amount of code in its wake. Newer code no longer follows the model that it established, but is, in fact, leaner, smaller, and uses far fewer classes and stored procedures. It's a joy to work with the new code. But I groan and cringe every time I have to wade through that older code. 

It was a case of very efficiently created code bloat. It would very likely have been stopped early on had someone been there to either (a) review the design of the code generator, (b) point me to an existing tool that was better at it, or (c) convince me that we actually had time to just do it the right way in the first place.

In any case, I'd never do it that way again. I have been duly chastised by my own foolishness.

I'm absolutely certain that I'm not the only one suffering from Vacuum Coding Syndrome. I'm equally certain that I need to have someone reviewing my designs and my code. So as I sat chain-smoking today, looking at a piece of paper with an abominably badly written piece of code printed on it that I knew I had to fix, I realized that I really needed to start submitting my designs for peer review to someone. Anyone, at this point, really. Because I really need to stop making silly mistakes. I'm the one that has to live with them.

So I got to thinking about it. In the past, I've submitted small snippets of code onto Usenet for review, and the response has been mixed. I've submitted some of my good code onto The Code Project and the response has actually been pretty good (but that code works). Now my brain is churning over a different kind of forum.

What I need is a forum where developers can submit code and/or designs for peer review. That would be the whole point of the forum. The online community is overflowing with professionals who are far more experienced, far more intelligent, and far more capable than I am. It would be a great boon to lone developers if there was a site designed specifically to allow us to post our code samples (sans IP) and designs (in some ubiquitous format) for peer review. 

I've seen products like CodeCollaborator, which foster collaborative code reviews and allow users to work together on code reviews using software. You can chat, compare notes, diff the files, and so on. Why hasn't anyone thought to do this on the Web? I'm not envisioning everything that CodeCollaborator does on the Web; what I am envisioning is a simple forum, like The Code Project or Construx's Software Best Practices.

So here I am, scouring the web, looking for such a resource. If any of you happen to know of one, I'd be much obliged if you could point it out to me. I'd roll my own, but given my history of making poor design decisions, and my lack of access to resources capable of reviewing my designs...

Well, you know. <shrug/>