For DRY or NOT DRY? To avoid code duplication and integrity

Question

I have a question about code duplication and refactoring, I hope that it is not too general. Let's say you have a rather small piece of code (~ 5 lines), which is a sequence of function calls, which is not a very low level. This code is repeated in several places, so it would probably be nice to extract the method here. However, in this particular example, this new function will suffer from low cohesion (which, among other things, manifests itself in the fact that it is difficult to find a good name for the function). The reason for this is probably because this repeating code is only part of a larger algorithm - and it is difficult to separate it into well-named steps.

What would you suggest in such a scenario?

Edit:

I wanted to leave the question at a general level so that more people could find it useful, but obviously it would be better to support it with some code. The example may not be the best of all (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(); } } } 

Answer 1

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

Suppose you dropped it. Think about what are the likely reasons for changing the resulting 5-line function. Would you most likely want to make changes that apply to all users, or do you have to write a new function that is slightly different from the old one, every time a caller has a reason to change?

If you want to change it for all users, this is a viable abstraction. Give him a poor name now, you can think of a better one later.

If you are going to eventually split this function into many similar versions, how your code will evolve in the future, this is probably not a viable abstraction. You can still write a function, but this is more of a "helper function" storing code than part of your official problem model. This is not very satisfactory: repeating this amount of code is a little worrying because it assumes that somewhere there must be a viable abstraction somewhere.

Perhaps 4 out of 5 lines can be abstracted, as they are more cohesive, and the fifth line just hangs around with them at the moment. Then you could write two new functions: one that is this new abstraction, and the other that is an assistant that calls a new function and then executes line 5. One of these functions may have a more useful life expectancy than the other ..

Answer 2

For me, the test of the litmus test: if I need to make changes to this code sequence in one place (for example, add a line, change the order), will I need to make the same change in other places?

If the answer is yes, then this is a logical "atomic" object and should be reorganized. If the answer is no, then the sequence of operations that work in more than one situation - and if refactoring is likely to cause you more problems in the future.

Answer 3

I’ve been thinking about this recently, and I understand exactly what you are getting at. It seems to me that this is an abstraction of the implementation more than anything in the world and, therefore, is more acceptable if you can avoid changing the interface. For example, in C ++, I can extract a function in cpp without touching the header. This somewhat reduces the need for the function abstraction to be well-formed and meaningful for the class user, since it is invisible to them until they need them (when added to the implementation).

Answer 4

For me, the operational word is “threshold”. Another word for this is likely to be "smell."

Things are always in balance. It seems (in this case) as the center of balance in cohesion (cool); and since you only have a small number of duplicates, this is not difficult.

If you had any major “event” and you switched to “1000” duplicates, then the balance would change, and so you can get closer.

For me, several managed duplicates are not a signal to the refactor (for now); but I will follow him.

Answer 5

Inheritance is your friend!

Do not duplicate the code. Even if one line of code is very long or complex, reconfigure it to a separate method with a distinguished name. Think of it as someone who will read your code in a year. If you call this function “blabla”, the next guy will find out what this function does without reading its code? If not, you need to change the name. After a week of such thinking, you will 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); } } }