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.

Recruitment email gone wrong

If you sign up to a recruitment site/agency you get called for ever. Even if they found you a job they'll call you in a year or so to make sure you're still happy or if not then they can help you find another role (and make some money for themselves). Anyway, you're meant to be friends with them so that you go back to them; I got an email this morning that didn't really warm me to the recruitment site:
Are you still working as NULL? Your profile was last updated 22/03/2004 and there are currently positions open that you may be missing out on.
 Perhaps they need some fresh programmers to work on their mass mailer?

Flickr, Mendhak and the full screen option

What's a blog for if not for self-promotion. I'm feeling altruistic too so I'm going to promote my friend Mendhak!
I've been a Flickr user for years; I love taking photos although I consider that I'm only a mediocre photographer. I love looking at portrait photos and Flickr was the first site I found that let me indulge in other people's photos in a rich way and with an abundance of photos. I've recently found 500px which has a nicer interface but you always prefer that first site, it'll take a while to port over to anything else.

Anyway, my friend Mendhak also shares a love of photography and of coding and has produced a signature generator and a full screen show-off-your-photo generator.
That's enough promotion for Mendhak, here's the self-promotion. Behold, leaves in autumn what I took (best viewed full screen on the biggest screen you own).

Thanks Mendhak : )

Friday, November 11, 2011

IEqualityComparer with Linq

I was completely thrown by a simple Linq issue today.

I was reading in two lists of strings (user names as it happens) and I wanted to know which ones from the second list weren't in the first list. Simple problem right?
First stab was to iterate over the second list; if FirstList.Contains(string) then I wasn't interested in that entry but if it didn't contain that entry I was interested in it and would squirrel it away.
Multiple runs of this code provided a different number of results despite the source data being the same. Confusion reigned for a bit, scratched head and poked the code a few times until I realised I wasn't telling the .Contains call how to figure out if one string was the same as the other.
That sounds simple but C# wasn't making any assumptions, I don't know what the default equality comparer was doing but providing my own implementation of IEqualityComparer<T> (T was a string in my case) solved this problem.

I love the satisfaction of solving a problem like this but good grief I felt a bit silly to be stumped by something so simple : )
For full closure I really should have a go at finding out what the default comparer actually does, no time at the moment though.

EDIT (12th November, 2011): Spent some more time on it. I haven't found what the default comparer does yet however Microsoft have already created StringComparers for me.
This is an abstract class that has a couple of static properties that expose the concrete examples. In my case I would have used StringComparer.OrdinalIgnoreCase as my comparer instead of writing my own.

Thanks Microsoft : )

First Post w00t w00t

A beginning is a very delicate time.
Know then that it is the year 2011 and this is the first post. Who can say how long I'll be posting, I may get bored and stop.

In any case, this is slightly more interesting than most first posts and I shall probably be the only one to read it whilst it's fresh.

See you later?