Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

To DRY or not to DRY? On avoiding code duplication and retaining cohesion

I've got a question concerning code duplication and refactoring, hope it's not too general. Say you've got a rather small piece of code (~5 lines) which is a sequence of function invocations that is - not a very low level. This code is repeated in several places, so it would probably be a good idea to extract a method here. However, in this particular example, this new function would suffer from low cohesion (which manifests itself, among others, by having a hard time finding a good name for the function). The reason for that is probably because this repeated code is just a part of a bigger algorithm - and it's difficult to divide it into well named steps.

What would you suggest in such scenario?

Edit:

I wanted to keep the question on a general level, so that more people can potentially find it useful, but obviously it would be best to back it up with some code sample. The example might not be the best one ever (it smells in quite a few ways), but I hope it does its job:

class SocketAction {

    private static class AlwaysCreateSessionLoginHandler extends LoginHandler {
        @Override
        protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException {
            Server.checkAllowedDeviceCount(socketAction._sess.getDeviceID());
            socketAction.registerSession();
            socketAction._sess.runApplication();
        }
    }

    private static class AutoConnectAnyDeviceLoginHandler extends LoginHandler {
        @Override
        protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException {
            if (Server.isUserRegistered(socketAction._sess.getUserLogin())) {
                Log.logSysInfo("Session autoconnect - acquiring list of action threads...");
                String[] sa = Server.getSessionList(socketAction._sess.getUserID());
                Log.logSysInfo("Session autoconnect - list of action threads acquired.");
                for (int i = 0; i < sa.length; i += 7) {
                    socketAction.abandonCommThreads();
                    Server.attachSocketToSession(sa[i + 1], socketAction._commSendThread.getSock());
                    return;
                }
            }
            Server.checkAllowedDeviceCount(socketAction._sess.getDeviceID());
            socketAction.registerSession();
            socketAction._sess.runApplication();
        }
    }

    private static class OnlyNewSessionLoginHandler extends LoginHandler {
        @Override
        protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException {
            socketAction.killOldSessionsForUser();
            Server.checkAllowedDeviceCount(socketAction._sess.getDeviceID());
            socketAction.registerSession();
            socketAction._sess.runApplication();
        }
    }
}
like image 832
lukem00 Avatar asked Nov 14 '10 15:11

lukem00


1 Answers

Question is too general to really say, but as an exercise:

Suppose you abstract it. Think about what the likely reasons are for wanting to change the resulting 5-line function. Would you want likely make changes that apply to all users, or would you end up having to write a new function that's slightly different from the old one, each time some caller has reason to want a change?

If you would want to change it for all users, it's a viable abstraction. Give it a poor name now, you might think of a better one later.

If you're going to end up splitting this function off into lots of similar versions as your code evolves in future, it's probably not a viable abstraction. You could still write the function, but it's more of a code-saving "helper function" than it is part of your formal model of the problem. This isn't very satisfactory: the repetition of this amount of code is a bit worrying, because it suggests there should be a viable abstraction in there somewhere.

Maybe 4 of the 5 lines could be abstracted out, since they're more cohesive, and the fifth line just so happens to be hanging around with them at the moment. Then you could write 2 new functions: one which is this new abstraction, and the other is just a helper that calls the new function and then executes line 5. One of these functions might then have a longer expected useful life than the other...

like image 115
Steve Jessop Avatar answered Nov 11 '22 14:11

Steve Jessop