Thursday, November 17, 2011

On the importance of good, readable code

Browsing through a solution I'm involved with I found a line that looks like this:
Utils.Reshape(xsltFileStream, out xslt);
It caught my eye because I have no idea what that method does based on that bit of code. It reshapes something (perhaps it reshapes the Util?), it receives a Stream and out's an XslCompiledTransform.
I could guess that it runs some xml through the xsl transform but it doesn't have any xml as a parameter and Utils is a class name, not an object (therefore making the method a static method, it doesn't use some xml from an instance variable).

So what does it do? Here's the method (I really should get a C# syntax highlighter...):
public static bool Reshape(Stream source, out XslCompiledTransform target)
    XmlTextReader xr = new XmlTextReader(source);
    target = new XslCompiledTransform();
    return (true);
So, it creates an XslCompiledTransform from the Stream you provide. Then it returns 'true'.
To be fair, this is very old code (I checked the source control history) and it's probably been changed (I didn't say refactored) a million times by different people to get into this state. There aren't any automated tests in this solution so it's scary to change code in this situation; countering that though is the fact that this code isn't in a library that's shared with anyone else and it's only called in one place in this solution.

What would I change?
  • That's a silly method name, 'LoadXslTransformerFromStream' is better but I bet there are even better names still. Descriptive names are great, descriptive and succinct get extra points but really long method names don't slow your code down so don't be afraid of them.
  • Returning a hard-coded value (that isn't used anyway) = FAIL. Make the method return an XslCompiledTransform, lovely.
  • Are utility classes really a good idea? I'm undecided but shouldn't they be encapsulated in a relevant object rather than floating as a static method in a helper class?
  • That cast to XmlReader could be improved - XmlTextReader is an XmlReader.

Anything else?

EDIT: Never mind a syntax highlighter, I'll try .NET Pad for a bit. Here's the Reshape method with some formatting.

No comments: