To DRY or not to DRY? On avoiding code duplication

问题:

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();
        }
    }
}

回答1:

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...

回答2:

To me, the litmus test is: if I need to make a change to this sequence of code in one place, (e.g. add a line, change the order), would I need to make the same change in other locations?

If the answer is yes, then it is a logical, "atomic" entity and should be refactored. If the answer is no, then it's a sequence of operations that happen to work in more than one situation - and if refactored, will likely cause you more trouble in the future.

回答3:

I was thinking about this lately and I understand exactly what you're getting at. It occurs to me that this is an implementation abstraction more than anything, and is thus more palatable if you can avoid changing an interface. For instance, in C++ I might extract the function into the cpp without touching the header. This somewhat lessens the requirement for the function abstraction to be well-formed and meaningful to the class user since it's invisible to them until they really need it (when adding to the implementation).

回答4:

For me the operative word is "threshold". Another word for this would probably be "smell".

Things are always in a balance. It sounds like (in this case) like the center of balance is in cohesiveness (cool); and as you've only got a small number of duplicates it's not hard to manage.

If you had some major "event" occur and you went to "1000" duplicates then the balance would have shiftyed and so might you're approach.

To me, a few managable duplicates isn't a signal to refactor (yet); but I'd keep an eye on it.

回答5:

Inheritance is Your friend!

Don't duplicate code. Even if a single line of code is very long or difficult, refactor it to a separate method with a distinctive name. Think of it like someone who will read Your code in a year. If You name this function "blabla", will the next guy know what this function does without reading it's code? If not, You need to change the name. After a week of thinking like that You'll get used to it and Your code will be 12% more readable! ;)

class SocketAction {

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

    private static class AlwaysCreateSessionLoginHandler extends CreateSessionLoginHandler;

    private static class AutoConnectAnyDeviceLoginHandler extends CreateSessionLoginHandler {
        @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;
                }
            }
            super.onLoginCorrect(socketAction);
        }
    }

    private static class OnlyNewSessionLoginHandler extends CreateSessionLoginHandler {
        @Override
        protected void onLoginCorrect(SocketAction socketAction) throws IllegalAccessException, IOException {
            socketAction.killOldSessionsForUser();
            super.onLoginCorrect(socketAction);
        }
    }
}