Избегайте чрезмерных параметров функции:подход, ориентированный на класс или функцию?

Question

Как бы вы исправили следующий неправильный код, который передает слишком много параметров?

void helper1(int p1, int p3, int p5, int p7, int p9, int p10) {
  // ...
}

void helper2(int p1, int p2, int p3, int p5, int p6, int p7, int p9, int p10) {
  // ...
}

void foo(int p1, int p2, int p3, int p4, int p5, int p6, int p7, int p8, 
         int p9, int p10) {
  helper1(p1, p3, p5, p7, p9, p10);
  helper2(p1, p2, p3, p5, p6, p7, p9, p10);
}

Я вижу два разных подхода:

Подход 1:Поместите все функции в класс

class Foo {
  private:
    int p1, p2, p3, p4, p5, p6, p7, p8, p9, p10;
    void helper1() {}
    void helper2() {}
  public:
    void foo() {
      helper1();
      helper2();
    }
    // add constructor
};

Подход 2:Просто передайте параметры как класс

struct FooOptions {
    int p1, p2, p3, p4, p5, p6, p7, p8, p9, p10;
};

void helper1(const FooOptions& opt) {
  // ...
}

void helper2(const FooOptions& opt) {
  // ...
}

void foo(const FooOptions& opt) {
  helper1(opt);
  helper2(opt);
}

Каковы преимущества и недостатки этих подходов?

Преимущество подхода 1 заключается в том, что - если вы сделаете helper функции виртуальные - вы можете создавать подклассы и перегружать их, добавляя гибкости.Но тогда, в моем случае (за исключением примера toy mini, который я привел) такие помощники часто шаблонны, поэтому они в любом случае не могут быть виртуальными.

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

(Это вопрос связан, но не обсуждает эти две альтернативы.)

Answer 1

Короткий ответ:

С Новым годом!Я бы избегал варианта № 1 и выбрал вариант № 2 только в том случае, если параметры могут быть разделены на четкие и логичные группы, которые имеют смысл вдали от вашей функции.

Длинный Ответ

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

calcTime(int p1, int p2, int p3, int p4, int p5, int p6) {
    dist = calcDistance( p1, p2, p3 );
    speed = calcSpeed( p4, p5, p6 );
    return speed == 0 : 0 ? dist/speed; }

Там вы можете сгруппировать вещи, чтобы быть более понятными, потому что существует четкое различие между параметрами.Тогда я бы предложил подход № 2.

С другой стороны, код, в котором мне передали, часто выглядит следующим образом:

calcTime(int p1, int p2, int p3, int p4, int p5, int p6) {
    height = height( p1, p2, p3, p6 );
    type = getType( p1, p4, p5, p6 );
    if( type == 4 ) {
        return 2.345; //some magic number
    }
    value = calcValue( p2, p3, type ); //what a nice variable name...
    a = resetA( p3, height, value );
    return a * value; }

что оставляет у вас ощущение, что эти параметры не совсем подходят для разделения на что-то значимое с точки зрения класса.Вместо этого вам было бы лучше подавать перевернутые вещи, такие как

calcTime(Type type, int height, int value, int p2, int p3)

а затем вызываю его

calcTime(
    getType( p1, p4, p5, p6 ),
    height( p1, p2, p3, p6 ),
    p3,
    p4 );

от которого у вас по спине могут пробежать мурашки, когда тоненький голосок в вашей голове кричит "СУХО, СУХО, СУХО!", Какой из них более читабелен и, следовательно, удобен в обслуживании?

Вариант № 1 мне в голову не приходит, так как существует очень большая вероятность, что кто-то забудет установить один из параметров.Это может очень легко привести к труднообнаруживаемой ошибке, которая проходит простые модульные тесты.ИММВ.

Answer 2

Я переписал классы и структуры, чтобы их можно было анализировать следующим образом:

class Point {
      private:
        int x, y
        void moveUp() {}
        void moveRight() {}
      public:
        void moveUpAndRight() {
          moveUp();
          moveRight();
        }
        // add constructor
    };

struct Point {
  int x, y;
};

void moveUp {}
void moveRight {}
void moveUpAndRight {
  moveUp {}
  moveRight {}
}

Использование классов позволяет вам воздействовать на ваш объект.Код, использующий объект point, будет использовать стандартный интерфейс для перемещения точки.Если система координат изменится и класс point будет изменен для определения системы координат, в которой он находится, это позволит использовать один и тот же интерфейс в нескольких системах координат (ничего не нарушая).В этом примере имеет смысл использовать подход 1.

Если вы просто группируете связанные данные для доступа в организационных целях...

struct Dwarves {
  int Doc, Grumpy, Happy, Sleepy, Bashful, Sneezy, Dopey;
}

void killDwarves(Dwarves& dwarves) {}
void buryDwarves(Dwarves& dwarves) {}

void disposeOfDwarves(Dwarves& dwarves) {
  killDwarves(dwarves);
  buryDwarves(dwarves);
}

Тогда имеет смысл перейти к подходу 2.

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

Answer 3

Если p1, p2 и т.д...на самом деле это просто параметры (флаги и т.д.), И они совершенно не связаны друг с другом, тогда я бы выбрал второй вариант.

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

class Something {
   int p1, p3, p5 ...;
   void helper1a();
};

class SomethingElse {
   int p7, p9, p10 ...;
   void helper1b();
};

class Options {
   int p2, p4, ...
};

void foo(Something &s, SomethingElse & else, Options &options) {
  helper2(options);
  s.helper1a();
  else.helper1b();
}

Answer 4

Вариант №2.

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

Затем вы можете создать экземпляры этого объекта и передать его в свою программу.

Обычно это может быть что-то вроде объекта приложения, объекта сеанса и т.д.

Answer 5

Подход к помещению их в класс "кажется" неправильным.Это похоже на попытку переделать старый код так, чтобы он выглядел как нечто, чем он не является.Со временем, возможно, удастся перейти к "объектно-ориентированному" стилю кода, но представляется более вероятным, что в конечном итоге это окажется неудачным сочетанием двух парадигм.Но это в основном основано на моих мыслях о том, что произошло бы с некоторым не-OO-кодом, с которым я работаю.

Таким образом, между этими двумя вариантами, я бы подумал, что вариант struct немного лучше.Это оставляет открытой гибкость для другой функции, которая может упаковать параметры в структуру и вызвать одну из вспомогательных функций.Но мне кажется, что у этого метода есть проблема с самодокументируемым аспектом.Если возникает необходимость добавить вызов Helper1 из какого-либо другого местоположения, не так ясно, какие параметры должны быть ему переданы.

Поэтому, хотя мне кажется, что в мою пользу идет ряд голосов "против", я думаю, что лучший способ - это ничего не менять.Тот факт, что хороший редактор покажет вам список параметров при вводе нового вызова одной из функций, делает его довольно простым для правильного выполнения.И стоимость, если только это не находится в зоне с интенсивным движением, не так уж велика.А в 64-разрядных средах это еще меньше, потому что большее количество параметров (это 4?) обычно передается в регистрах.

Здесь это связанный с этим вопрос, говорящий об этом с участием большого количества респондентов.

Answer 6

Я не знаю.Это действительно зависит от того, какие параметры являются.Нет никакого волшебного шаблона проектирования "заставить 20 параметров функции исчезнуть", который вы могли бы вызвать.Лучшее, что вы можете сделать, это провести рефакторинг вашего кода.В зависимости от того, что представляют параметры, может иметь смысл сгруппировать некоторые из них в класс параметров, или поместить все в один монолитный класс, или разделить все это на несколько классов.Один из вариантов, который может сработать, - это некоторая форма каррирования, при которой объекты передаются понемногу, каждый раз выдавая новый объект, для которого может быть вызван следующий вызов, передавая следующий пакет параметров.Это особенно имеет смысл, если некоторые параметры часто остаются неизменными во всех вызовах.

Более простым вариантом этого может быть перенос всего в класс, где некоторые параметры передаются в конструкторе (те, которые обычно остаются неизменными во всем пакете вызовов), а другие, те, которые должны предоставляться при каждом вызове, поскольку они постоянно меняются, передаются при фактическом вызове функции (вероятно, перегруженном operator()).

Не зная действительного значение что касается вашего кода, невозможно сказать, как его лучше всего реорганизовать.

Answer 7

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