Friday, September 10, 2010

Adventures in Repetitive Code

One of the things you often see in legacy systems is repetitive little blocks of code. Take this example from Suteki Shop, here we’re inserting some static data into the database to setup the initial set of roles:

static void InsertRoles1(ISession session)
{
    var admin = new Role { Id = 1, Name = "Administrator" };
    session.Save(admin);
    
    var orderProcessor = new Role { Id = 2, Name = "Order Processor" };
    session.Save(orderProcessor);
    
    var customer = new Role { Id = 3, Name = "Customer" };
    session.Save(customer);
    
    var guest = new Role { Id = 4, Name = "Guest" };
    session.Save(guest);
}

The only thing that varies in these four code blocks is the id and the name. One way of factoring this out would be to write a little nested private class:

private class RoleInserter
{
    private readonly ISession session;

    public RoleInserter(ISession session)
    {
        this.session = session;
    }

    public void Insert(int id, string name)
    {
        var role = new Role { Id = id, Name = name };
        session.Save(role);
    }
}

So our InsertRoles method now looks like this:

static void InsertRoles1(ISession session)
{
    var roleInserter = new RoleInserter(session);

    roleInserter.Insert(1, "Administrator");
    roleInserter.Insert(2, "Order Processor");
    roleInserter.Insert(3, "Customer");
    roleInserter.Insert(4, "Guest");
}

But now we’ve actually got more code in total than before. Also it’s one of the unfortunate things about object oriented programming that simple utility functions like this are awkward. The silly name of the class ‘RoleInserter’ is a dead giveaway. Steve Yegge calls it The Kingdom of Nouns. We don’t really need a whole class, we just need a single function.

Let’s use a closure to replace RoleInserter with a function:

private static Action<int, string> GetRoleInserter(ISession session)
{
    return (id, name) =>
    {
        var role = new Role { Id = id, Name = name };
        session.Save(role);
    };
}

Now our InsertRoles method looks like this:

static void InsertRoles1(ISession session)
{
    var insertRole = GetRoleInserter(session);

    insertRole(1, "Administrator");
    insertRole(2, "Order Processor");
    insertRole(3, "Customer");
    insertRole(4, "Guest");
}

There’s little need to factor out the GetRoleInserter, it’s simpler just to write it in line:

static void InsertRoles(ISession session)
{
    Action<int, string> insertRole = (id, name) =>
    {
        var role = new Role {Id = id, Name = name};
        session.Save(role);
    };

    insertRole(1, "Administrator");
    insertRole(2, "Order Processor");
    insertRole(3, "Customer");
    insertRole(4, "Guest");
}

That’s much nicer. Using lambdas like this can really clean up little repetitive code blocks.

10 comments:

Ben Taylor said...

While I'm a big fan of Action and Func code, an extension might be nicer. It would remove the need to specify the ID and could be easily reused in other tests (as well as being understood by Action/Func fearful devs).

Something like below (typed in the comment, may not compile :)...


static void InsertRoles(ISession session)
{
session.SaveNamedRoles("Administrator", "Order Processor", "Customer", "Guest");
}

public static class TestSessionExtensions
{
public static void InsertNamedRoles(this Isession session, params string[] roles)
{
//TODO loop roles and save each (just increment id)
}
}

Ben Taylor said...

Obviously, the extension method InsertNamedRoles should be called SaveNamedRoles.

Mike Hadlow said...

Hi Ben,

Yes, if reuse is the aim then the inline closure is not the way to go.

As for these Action/Func fearful devs, do they actually exist? I've worked with people who hadn't seen/used lambdas before, but they got up to speed pretty fast.

Ben Taylor said...

Yes, they certainly exist. IME there is no problem when someone (e.g. you or me) is around to help them get them up to speed. However, I have seen problems on larger (50+), co-located projects where skill levels are, er, varied.

Don't get me wrong. I have a rep on my current project for my (over)love of lambda :) Personally, I think everyone should know and love Action & Func. I am using them in place of strategy classes etc a lot these days.

The example here just looks a bit awkward to me. I think it's the need to duplicate the Action call and manually increment the IDs. That still looks like duplication to me. The extension method looks DRY-er regardless of reuse requirements.

fschwiet said...

If someone doesn't get lambda expressions, they're likely the type that abuses extension methods if they use them at all. I think the inline lambda is pretty clean, it isolates the function to where its used.

If this is something thats going to be reused broadly, then I think I'd prefer the insert roles class as opposed to an extension method, as then I have options to replace the dependency when testing.

Its really just preferences though, any of them are reasonable.

Mike Hadlow said...

Ben,

Maybe it's not a very good example. I guess the point I wanted to make was that you should try to remove 'boiler plate' code whenever possible. Look at what is different in otherwise similar blocks and boil it down to that. It's kinda accidental that the ids in this case are incremental, maybe I should have made them 4, 7, 11 and 243 instead :)

I feel your pain, the happy place for any developer is when he's the dumbest guy in the room. Working in any team where you are encouraged to dumb down your code so that other people can understand it can be really fustrating.

Chris said...

"the happy place for any developer is when he's the dumbest guy in the room."

Aint that the truth?!

I don't think passing ISession around all over the place is good. Seems like a leak in abstractions to me. I think what you're looking for is the Unit of Work pattern.

I know that ISession is basically that. But hard-coupling to NH smells bad to me. :)

Ben Taylor said...

@fschwiet @Mike - I think we are all right! But Mike is obviously the most right. At the end of the day it's all about what works for your project and the people/plans you have.

Like Mike's version, I feel we now have closure...

Steve Smith said...

I wrote something similar a while back:
http://stevesmithblog.com/blog/eliminate-repetition-with-action-lt-t-gt/

I like the in-lining of the action, though. Very nice.

Mike Hadlow said...

Hi Chris,

Agreed, but this is a little utility to insert static data. I didn't think it waranted fully encapsulating NH.

Hi Steve,

That's very nice. Higher-order functions really do lead to some nice refactorings. It's a shame you can't put extension methods on methods:

MyMethod.MyExtensionMethod();

Would be really cool. This works though:

((Action)MyMethod).MyExtensionMethod();

but it's ugly :(