Одинаковые куски кода в разных классах допустимо или нубство?

Question

[Chvalov]

Данный кусок кода используется в двух разных классах (MainActivity.java и Response.java):

// Подсчет тока по каждому двигателю
    private float getCurrent(int engine){
        float phasesA = calculatePhaseForEngine(0, MassCurrentsPhases, engine);
        float phasesB = calculatePhaseForEngine(1, MassCurrentsPhases, engine);
        float phasesC = calculatePhaseForEngine(2, MassCurrentsPhases, engine);

        return (phasesA + phasesB + phasesC) / 3;
    }
    // Подсчетываем ток по выбраному двигателю
    private static float calculatePhaseForEngine(int i, float[] MassCurrentsPhases, int currentEng) {
        return MassCurrentsPhases[3 * currentEng + i];
    }

Также есть и вод такие участки кода в одном классе (Response) используются в цикле:

switch (data.length) {
            case 96:    // Работа со статусом
                for (int i = 0, j = 0; i < 27; i++, j += 2) {
                    FirstByteTmp = (0x000000FF & ((int) data[21 + j]));
                    SecondByteTmp = (0x000000FF & ((int) data[20 + j]));
                    ByteTmp = (char) (FirstByteTmp << 8 | SecondByteTmp);
                    MassCurrentsPhases[i] = round((float) ByteTmp / 100, 2);
                }
                // .................................
                //..................................
case 71:    // Ковыряем ответ с длиной в 71 значение
                if(data[2] == 0){   // Смотрим 2 значение и если там 0 выполняем код ниже
                    for (int i = 0, j = 0; i < 30; i++, j += 2) {   // Записываем даные в массив
                        FirstByteTmp = (0x000000FF & ((int) data[5 + j]));
                        SecondByteTmp = (0x000000FF & ((int) data[4 + j]));
                        ByteTmp = (char) (FirstByteTmp << 8 | SecondByteTmp);
                       // .................................
                       //..................................
                       } else if(data[2] == 1){ // Смотрим 2 значение и если там 1 выполняем код ниже
                    for (int i = 0, j = 0; i < 30; i++, j += 2) {   // Записываем даные в массив
                        FirstByteTmp = (0x000000FF & ((int) data[5 + j]));
                        SecondByteTmp = (0x000000FF & ((int) data[4 + j]));
                        ByteTmp = (char) (FirstByteTmp << 8 | SecondByteTmp);

Допустимо ли дублирующие участки кода в ООП и можно ли как то эти дубли убрать без лишних костылей ?

Comments

[amf1k]

Наследование?

Answer 1

[dude2012]

Дело даже не нубстве. А в практической составляющей. Лучше один код исправить, чем два. Но на самом деле это не так страшно, как забыть исправить второй код, если исправил первый.. Поэтому, чем меньше исправлений, тем лучше - меньше головной боли, меньше потенциальных ошибок. Грабли могут сильно ударить по голове, не в этот раз, так в следующий. Лучше иметь хорошие привычки.
Рекомендую "Рефакторинг" Мартина Фаулера почитать, там много примеров на эту тему.
Как раз убрать дубли - это рефакторинг.

Comments

[Chvalov]

dude2012 Спасибо большое, книгу мне уже советовали ранее но что-то времени к ней добраться не было.

Answer 2

[Сергей Протько]

нубство. Выносите общее в методы, в общие зависимости. Почитайте про DRY.

Дублирование позволительно если это... ну тупой код и от дублирования никуда не деться. Но у вас тут какие-то выражения, логика, поведение дублируется. А это уже нубство.

Comments

[Chvalov]

Сергей Протько Можете подсказать как быть с данными кусками кода ?

for (int i = 0, j = 0; i < 27; i++, j += 2) {
                    FirstByteTmp = (0x000000FF & ((int) data[21 + j]));
                    SecondByteTmp = (0x000000FF & ((int) data[20 + j]));
                    ByteTmp = (char) (FirstByteTmp << 8 | SecondByteTmp);

И

for (int i = 0, j = 0; i < 30; i++, j += 2) {   // Записываем даные в массив
                        FirstByteTmp = (0x000000FF & ((int) data[5 + j]));
                        SecondByteTmp = (0x000000FF & ((int) data[4 + j]));

[Денис Загаевский]

Chvalov: вынести в отдельный метод, очевидно.

[Сергей Протько]

Chvalov: как и сказал Denis Zagayevskiy, в отдельный метод вынести, желательно приватный.

Answer 3

[Evhen]

По поводу "switch (data.length)": тут нужно задействовать полиморфизм. Создать интерфейс с методом который принимает массив int[]data, и под каждый вариант case сделать соответствующую реализацию.

Например

interface Worker {
  void doWork(int[] data);
}

class Worker96 implements Worker  {
   public void doWork(int[] data) {
       ....
   }
}

class Worker71 implements Worker  {
   public void doWork(int[] data) {
       ....
   }
}

// потом можно создать мапу 

class Main {

private Map<Integer, Worker> workers = ....;

Main (){
    workers.put(96, new Worker96());
    workers.put(71, new Worker71());
}
public void dataProcessing(int[] data) {
   Worker w = workers.get(data.length);
   if (w == null)
       throw new RuntimeException("Unsupported data length!");

   w.doWork(data);
}
}
// как то так...

По поводу первого случая, если код не предполагает хранение состояния,
то можно вынести код в отдельный утильный класса в статический метод.

Comments

[Chvalov]

Спасибо, только я работаю с не с массивом интов, а с байтами и битами.
с полиморфизм еще толком не знаком :( (Учусь пока)

[Evhen]

Chvalov: А не важно, главное чтоб вы поняли идею. вместо интового массива, укажите байтовый, смысл от этого не изменится.

[Михаил]

Eugene: не нарушает ли такая реализация LSP?

[Evhen]

Михаил: ну... скорее всего да

Answer 4

[Михаил]

В чистом коде дублирования не бывает, от него всегда можно избавиться, но, все зависит только от времени.
Есть ситуации, когда для избавления от дублирования нужно полностью перепроектировать проект или лепить костыли. Чаще всего происходит так:
- Приходит босс, говорит сделать задачу, сделать надо вчера
- Ты смотришь в код и видишь, что уже есть часть решения, нужно его использовать в другом месте, радуешься
- Пытаешься использовать, понимаешь, чтобы прокинуть этот код, нужно написать с десяток классов и переделать пол проекта, расстраиваешься, за день явно не сделать, сроки поджимают
- Один из 2-х вариантов, или пилишь костыль чтобы код таки был в одном месте, или тупо копируешь его))
- Готово

Такое бывает, чаще всего, с legacy кодом

Answer 5

[Дмитрий Александров]

Сильно рекомендую почитать "Паттерны проектирования" (O'relly Эрик и Элизабет Фриман), очень много подобных вопросов исчезнет сразу и писать будет намного проще.
Для Вашей ситуации предусмотрены абстрактные классы. К примеру:

public abstract class MyClass{
//Ваши общие методы реализованны тут
// Подсчет тока по каждому двигателю
    private float getCurrent(int engine){
        float phasesA = calculatePhaseForEngine(0, MassCurrentsPhases, engine);
        float phasesB = calculatePhaseForEngine(1, MassCurrentsPhases, engine);
        float phasesC = calculatePhaseForEngine(2, MassCurrentsPhases, engine);

        return (phasesA + phasesB + phasesC) / 3;
    }
    // Подсчетываем ток по выбраному двигателю
    private static float calculatePhaseForEngine(int i, float[] MassCurrentsPhases, int currentEng) {
        return MassCurrentsPhases[3 * currentEng + i];
    }
public abstract void doSomething(); //Такие методы наследники будут обязаны реализовать
}

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