Передача переменной-члена в качестве параметра метода

33

В проекте я нашел такой код:

class SomeClass
{
    private SomeType _someField;

    public SomeType SomeField
    {
        get { return _someField; }
        set { _someField = value; }
    }

    protected virtual void SomeMethod(/*...., */SomeType someVar)
    {
    }

    private void SomeAnotherMethod()
    {
        //.............
        SomeMethod(_someField);
        //.............
    }

};

Как мне убедить моих товарищей по команде, что это плохой код?

Я считаю, что это ненужное осложнение. Зачем передавать переменную-член в качестве параметра метода, если у вас уже есть доступ к ней? Это тоже нарушение инкапсуляции.

Видите ли вы другие проблемы с этим кодом?

ТИКА
источник
21
Что заставляет вас думать, что это плохо?
Яннис
@ Яннис Ризос, ты думаешь, это хорошо? По крайней мере, это ненужное осложнение. Зачем передавать переменную как параметр метода, если у вас уже есть доступ к ней? Это также нарушение инкапсуляции.
Тика
2
Действительные точки, пожалуйста, измените вопрос, чтобы включить их. Мы не можем помочь вам убедить своих товарищей по команде, но мы можем помочь вам оценить код, и именно об этом должен быть ваш вопрос.
Яннис
8
Я предпочел бы иметь метод, который может суммировать различные переменные, чем метод, который делает константу 2 + 2. Параметр метода предназначен для повторного использования.
Данте
Важным моментом здесь является тип этого параметра. Если это ссылочный тип, я не вижу никакого преимущества, но если это тип значения, я думаю, что это имеет смысл, поскольку, если вы модифицируете этот тип переменной, компилятор предупредит вас о том месте, где вы нарушили код.
Реми

Ответы:

3

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

Я бы порекомендовал сделать шаг назад и вместо того, чтобы сосредоточиться именно на передаче элементов в качестве параметров, начать обсуждение с вашей командой ясности и читабельности. Либо более формально, либо просто в коридорах, обсудите, что делает некоторые сегменты кода более легкими для чтения, а другие - более сложными. Затем определите показатели качества или атрибуты чистого кода, к которым вы, как команда, хотели бы стремиться. В конце концов, даже работая над «зелеными» проектами, мы тратим более 90% времени на чтение, и как только код написан (скажем, через 10–15 минут), он уходит в техобслуживание, где читаемость становится еще важнее.

Так что для вашего конкретного примера здесь я бы использовал аргумент, что меньше кода всегда легче прочитать, чем больше кода. Функция, которая имеет 3 параметра, для мозга труднее обрабатывать, чем функция, которая не имеет ни одного параметра или 1 параметра. Если есть другое имя переменной, мозг должен отслеживать еще одну вещь при чтении кода. Итак, чтобы запомнить «int m_value», а затем «int localValue» и помнить, что одно действительно означает, что другое всегда дороже для вашего мозга, чем просто работа с «m_value».

Для большего количества боеприпасов и идей я бы порекомендовал взять копию Чистого кода дяди Боба .

DXM
источник
Проголосовал, увидев влияние на книгу, на которую ссылаются.
Фрэнк Хайлеман
Хотя небольшая часть меня очень огорчена тем, что через 5 лет после написания ответа вы просто отобрали у меня 2 интернет-пункта, есть еще одна маленькая часть меня, которая любопытна, если бы вы могли предоставить некоторые ссылки, которые будут свидетельствовать о (предположительно, плохом) влияние, которое имела книга, на которую я ссылался. Это казалось бы справедливым и почти стоящим этих очков
ДХМ
Ссылка на меня, лично видя влияние на других разработчиков. Все мотивы, стоящие за такими книгами, хороши, однако, указав, что весь код должен следовать нестандартным рекомендациям (т. Е. Рекомендациям, которые фактически служат цели), такие книги порождают потерю критического мышления, которое некоторые интерпретируют как культовое. ,
Фрэнк Хилман
Что
30

Я могу придумать оправдание для передачи поля члена как параметра в (приватном) методе: он делает явным то, от чего зависит ваш метод.

Как вы говорите, все поля-члены являются неявными параметрами вашего метода, потому что весь объект является. Однако нужен ли полный объект для вычисления результата? Если SomeMethodэто внутренний метод, который зависит только от того _someField, не чище ли сделать эту зависимость явной? Фактически, если сделать эту зависимость явной, это также может указывать на то, что вы на самом деле можете рефакторировать этот кусок кода из своего класса! (Обратите внимание, я предполагаю, что мы говорим здесь не о методах получения или установки, а о коде, который фактически что-то вычисляет)

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

Андрес Ф.
источник
2
Какой будет оставшаяся неявная зависимость? Из вашего примера я предположил, что _someFieldэто единственный параметр, необходимый для вычисления результата, и мы просто сделали его явным. (Примечание: это важно. Мы не добавили зависимость, мы только сделали ее явной!)
Andres F.
11
-1 Если не существует неявных зависимостей от членов экземпляра, то это должен быть статический член, который принимает значение в качестве параметра. В этом случае, хотя у вас есть причина, я не думаю, что это обоснованное оправдание. Это плохой код.
Стивен Эверс
2
@SnOrfus Я действительно предложил рефакторинг метода полностью вне класса
Андрес Ф.
5
+1. для "... не чище ли сделать эту зависимость явной?" Абсо-долбанные-loutely. Кто здесь сказал бы, что "глобальные переменные хороши как общее правило"? Это так Кобол-68. Услышь меня сейчас и поверь мне позже. В нашем нетривиальном коде я иногда буду рефакторировать, чтобы явно указать, где используются глобальные переменные класса. Во многих случаях мы облажались с помощью а) произвольного обратного использования частного поля и его открытого свойства, б) запутывания преобразования полей путем «скрытия зависимостей». Теперь умножьте это на 3-5 глубоких цепочек наследования.
radarbob
2
@Tarion Я не согласен с дядей Бобом в этом. По возможности, методы должны быть функциональными и зависеть только от явных зависимостей. (При вызове открытых методов в ООП одной из этих зависимостей является this(или self), но это делается явно самим вызовом obj.method(x)). Другие неявные зависимости - это состояние объекта; это обычно затрудняет понимание кода. По возможности - и в пределах разумного - делайте зависимости явными и функциональными. В случае частных методов, если возможно, явно передавайте каждый параметр, который им нужен. И да, это помогает изменить их.
Андрес Ф.
28

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

Конечно, это не просто и не практично, чтобы все ваши методы были чистыми методами на языке ООП. Но я полагаю, что с точки зрения ясности кода вы значительно выиграете, если будете иметь методы, которые обрабатывают сложную логику, и использовать неизменяемые переменные, сохраняя при этом нечистую «глобальную» обработку состояний в отдельных выделенных методах.

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

scrwtp
источник
5
Супур ответь. Несмотря на идеал hwats, многие классы в современном программном обеспечении больше и уродливее, чем целые программы, когда была придумана фаза «Globals is Evil». Переменные класса для всех практических целей являются глобальными в рамках экземпляра класса. Выполнение большого объема работы в чистых функциях делает код намного более тестируемым и надежным.
Mattnz
@mattnz, есть ли способ дать ссылку на библиографию Хвата или любую из его книг об идеальном программировании? Я искал в интернете и ничего не могу найти на нем. Google продолжает пытаться автоматически исправить это на "что".
Баттл Буткус
8

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

if ( CalculateCharges(newStartDate) > CalculateCharges(m_StartDate) )
{
     //handle increase in charges
}

где newStartDateнекоторая локальная переменная и m_StartDateявляется переменной-членом.

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

Кейт Грегори
источник
3
Неважно, что метод вызывается с параметрами, отличными от переменной-члена. Важно то, что это можно так назвать. Вы не всегда знаете, как метод будет в конечном итоге вызываться при его создании.
Калеб
Возможно, вам лучше заменить условие «if» на «needHandleIncreaseChages (newStartDate)», и тогда ваш аргумент больше не будет выполняться.
Тарион
2

Что никто не затронул, так это то, что SomeMethod защищен виртуально. Это означает, что производный класс может использовать его и повторно реализовать его функциональность. Производный класс не будет иметь доступа к закрытой переменной и, следовательно, не сможет обеспечить пользовательскую реализацию SomeMethod, которая зависит от закрытой переменной. Вместо того, чтобы брать зависимость от закрытой переменной, объявление требует, чтобы вызывающий передавал ее.

Майкл Браун
источник
Чего вам не хватает, так это того, что эта закрытая переменная-член имеет открытые средства доступа
Тика
То есть вы говорите, что защищенный виртуальный метод должен зависеть от общедоступного метода доступа к закрытой переменной? А у вас есть проблемы с текущим кодом?
Майкл Браун
Публичные средства доступа созданы для использования. Период.
Тика
1

Фреймворки GUI обычно имеют своего рода класс «View», который представляет вещи, нарисованные на экране, и этот класс обычно предоставляет метод, подобный тому, invalidateRect(Rect r)чтобы пометить часть его области рисования как нуждающуюся в перерисовке. Клиенты могут вызывать этот метод для запроса обновления части представления. Но представление может также вызвать свой собственный метод, например:

invalidateRect(m_frame);

вызвать перерисовку всей области. Например, это может быть сделано при первом добавлении в иерархию представлений.

В этом нет ничего плохого - рамка представления является допустимым прямоугольником, и само представление знает, что оно хочет перерисовать себя. Класс View может предоставлять отдельный метод, который не принимает параметров и использует вместо этого фрейм представления:

invalidateFrame();

Но зачем добавлять специальный метод именно для этого, когда вы можете использовать более общий invalidateRect()? Или, если вы решили предоставить invalidateFrame(), вы, скорее всего, реализовали бы его с точки зрения более общего invalidateRect():

View::invalidateFrame(void)
{
    invalidateRect(m_frame)
}

Зачем передавать переменную как параметр метода, если у вас уже есть доступ к ней?

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

Калеб
источник
1

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

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

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

Джеймс Андерсон
источник
1

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

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

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

S.Robins
источник
1

Некоторые вещи, которые приходят ко мне, когда я думаю об этом:

  1. В общем, сигнатуры методов с меньшим количеством параметров легче понять; большая причина, по которой были изобретены методы, заключалась в том, чтобы исключать длинные списки параметров, объединяя их с данными, с которыми они работают.
  2. Привязка сигнатуры метода к переменной-члену усложнит изменение этих переменных в будущем, потому что вам нужно не только изменять внутри метода, но и везде, где метод также вызывается. А поскольку SomeMethod в вашем примере защищен, подклассы также необходимо будет изменить.
  3. Методы (публичные или частные), которые не зависят от внутренних классов, не обязательно должны быть в этом классе. Они могут быть включены в служебные методы и быть такими же счастливыми. Они почти не имеют бизнеса, будучи частью этого класса. Если никакой другой метод не зависит от этой переменной после перемещения метода (ов), то эта переменная тоже должна идти! Скорее всего, переменная должна быть в своем собственном объекте, и этот метод или методы, работающие с ней, становятся открытыми и составляются родительским классом.
  4. Передача данных в различные функции, такие как ваш класс, является процедурной программой с глобальными переменными, которая просто бросает вызов ОО-дизайну. Это не цель переменных-членов и функций-членов (см. Выше), и это звучит так, как будто ваш класс не очень сплочен. Я думаю, что это запах кода, который предлагает лучший способ сгруппировать ваши данные и методы.
Колин Хант
источник
0

Вы пропускаете, если параметр ("аргумент") является ссылкой или только для чтения.

class SomeClass
{
    protected SomeType _someField;
    public SomeType SomeField
    {
        get { return _someField; }

        set {
          if (doSomeValidation(value))
          {
            _someField = value;
          }
        }
    }

    protected virtual void ModifyMethod(/*...., */ ref SomeType someVar)
    { 
      // ...
    }    

    protected virtual void ReadMethod(/*...., */ SomeType someVar)
    { 
      // ...
    }

    private void SomeAnotherMethod()
    {
        //.............

        // not recommended, but, may be required in some cases
        ModifyMethod(ref this._someField);

        //.............

        // recommended, but, verbose
        SomeType SomeVar = this.someField;
        ModifyMethod(ref SomeVar);
        this.someField = SomeVar;

        //.............

        ReadMethod(this.someField);
        //.............
    }

};

Многие разработчики, как правило, напрямую присваивают внутренние поля переменной в методах конструктора. Есть несколько исключений.

Помните, что «сеттеры» могут иметь дополнительные методы, а не просто присваивание, а иногда даже виртуальные методы или вызов виртуальных методов.

Примечание: я предлагаю сохранить внутренние поля свойств как «защищенные».

umlcat
источник