СУХОЙ или НЕ СУХОЙ? Об избежании дублирования кода и сохранении сплоченности

Question

У меня вопрос по поводу дублирования кода и рефакторинга, надеюсь, он не слишком общий.Скажем, у вас есть довольно маленький кусок кода (~ 5 строк), который представляет собой последовательность вызовов функций , то есть - не очень низкий уровень .Этот код повторяется в нескольких местах, поэтому, вероятно, было бы неплохо извлечь метод здесь.Однако в этом конкретном примере эта новая функция страдает от низкой сплоченности (что проявляется, в частности, в том, что трудно найти хорошее имя для функции).Причина этого, вероятно, в том, что этот повторяющийся код является лишь частью более крупного алгоритма - и его трудно разделить на хорошо именованные шаги.

Что бы вы предложили в таком сценарии?

Редактировать:

Я хотел сохранить вопрос на общем уровне, чтобы потенциально больше людей могло найти его полезным, но, очевидно, было бы лучше подкрепить его некоторым примером кода.Этот пример может быть не самым лучшим (он пахнет довольно многими способами), но я надеюсь, что он справится со своей задачей:

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

Вопрос слишком общий, чтобы на самом деле сказать, но в качестве упражнения:

Предположим, вы абстрагируете его.Подумайте о том, каковы вероятные причины желания изменить результирующую 5-строчную функцию.Вероятно, вы захотите внести изменения, применимые ко всем пользователям, или вам придется написать новую функцию, немного отличающуюся от старой, каждый раз, когда у какого-то вызывающего абонента появляется причина для изменения?

Если выхотел бы изменить его для всех пользователей, это жизнеспособная абстракция.Дайте ему плохое имя сейчас, вы можете подумать о лучшем позже.

Если вы собираетесь в конечном итоге разделить эту функцию на множество похожих версий по мере развития вашего кода в будущем, вероятно, это нежизнеспособная абстракция.Вы все еще могли бы написать функцию, но это скорее сохраняющая код «вспомогательная функция», чем часть вашей формальной модели проблемы.Это не очень удовлетворительно: повторение такого количества кода немного беспокоит, потому что оно предполагает, что должно быть жизнеспособной абстракцией где-то там.

Возможно 4 из 5 строкможно было бы абстрагироваться, так как они более сплоченные, и пятая строка в данный момент так и осталась с ними.Затем вы могли бы написать 2 новые функции: одна - это новая абстракция, а другая - просто помощник, который вызывает новую функцию и затем выполняет строку 5. Одна из этих функций может иметь более ожидаемый срок полезного использования, чем другая..

Answer 2

Для меня лакмусовая бумажка: если мне нужно внести изменения в эту последовательность кода в одном месте (например, добавить строку, изменить порядок), нужно ли мне внести такое же изменение в другие места?

Если ответ «да», то это логическая «атомарная» сущность, и ее следует реорганизовать. Если ответ «нет», то это последовательность операций, которая может работать более чем в одной ситуации - и, если ее подвергнуть рефакторингу, скорее всего, доставит вам больше проблем в будущем.

Answer 3

Для меня оперативное слово «порог».Другое слово для этого, вероятно, будет «запах».

Вещи всегда в равновесии.Похоже (в данном случае), что центр баланса находится в связности (круто);и так как у вас есть только небольшое количество дубликатов, управлять не сложно.

Если бы у вас произошло какое-то серьезное «событие» и вы пошли на «1000» дубликатов, то баланс изменился бы, и поэтомувы подходите.

Для меня несколько управляемых дубликатов не является сигналом для рефакторинга (пока);но я бы за этим присматривал.

Answer 4

В последнее время я думал об этом и прекрасно понимаю, к чему вы клоните.Мне приходит в голову, что это абстракция реализации больше всего на свете, и поэтому она более приемлема, если вы можете избежать изменения интерфейса.Например, в C ++ я мог бы извлечь функцию в cpp, не касаясь заголовка.Это несколько уменьшает необходимость того, чтобы абстракция функции была правильно сформирована и содержала смысл для пользователя класса, поскольку он невидим для них, пока он им действительно не нужен (при добавлении в реализацию).

Answer 5

Наследование - Ваш друг!

Не дублируйте код.Даже если одна строка кода очень длинная или сложная, реорганизуйте ее в отдельный метод с отличительным именем.Думайте об этом как о ком-то, кто прочитает Ваш код через год.Если вы назовете эту функцию «blabla», следующий парень узнает, что эта функция делает, не читая ее код?Если нет, вам нужно изменить имя.После недельного размышления вы привыкнете к этому, и ваш код станет 12% более читабельным!;)

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