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

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

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 функции виртуальными - вы можете создавать подклассы и перегружать их, добавляя гибкости. Но в моем случае (помимо приведенного мной мини-примера игрушек) такие помощники часто являются шаблонными, поэтому они все равно не могут быть виртуальными.

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

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


person Frank    schedule 01.01.2010    source источник


Ответы (7)


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

С новым годом! Я бы избегал варианта №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 в моей голове не подходит, так как очень велика вероятность, что кто-то забудет установить один из параметров. Это может очень легко привести к трудно обнаруживаемой ошибке, которая проходит простые модульные тесты. YMMV.

person wheaties    schedule 01.01.2010

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

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 {}
}

Использование классов позволяет вам воздействовать на ваш объект. Код, использующий точечный объект, будет использовать стандартный интерфейс для перемещения точки. Если система координат изменится и класс точки будет изменен для определения системы координат, в которой она находится, это позволит использовать один и тот же интерфейс в нескольких системах координат (ничего не нарушая). В этом примере имеет смысл использовать подход 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.

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

person Derek Litz    schedule 01.01.2010

Если 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();
}
person phtrivier    schedule 01.01.2010

Вариант №2.

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

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

Обычно это может быть что-то вроде объекта Application, объекта Session и т. Д.

person Larry Watanabe    schedule 01.01.2010

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

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

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

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

person Mark Wilkins    schedule 01.01.2010

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

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

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

person jalf    schedule 01.01.2010

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

person Ozan    schedule 01.01.2010