В проекте я нашел такой код:
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);
//.............
}
};
Как мне убедить моих товарищей по команде, что это плохой код?
Я считаю, что это ненужное осложнение. Зачем передавать переменную-член в качестве параметра метода, если у вас уже есть доступ к ней? Это тоже нарушение инкапсуляции.
Видите ли вы другие проблемы с этим кодом?
Ответы:
Я думаю, что это актуальная тема, но причина, по которой вы получаете неоднозначные ответы, заключается в том, как был сформирован вопрос. Лично у меня был такой же опыт с моей командой, когда передача членов в качестве аргументов была ненужной и запутывала код. У нас был бы класс, который работает с набором членов, но некоторые функции обращаются к членам напрямую, а другие функции изменяют одни и те же члены через параметры (то есть используют совершенно разные имена), и для этого не было абсолютно никаких технических причин. По технической причине я имею в виду пример, который привела Кейт.
Я бы порекомендовал сделать шаг назад и вместо того, чтобы сосредоточиться именно на передаче элементов в качестве параметров, начать обсуждение с вашей командой ясности и читабельности. Либо более формально, либо просто в коридорах, обсудите, что делает некоторые сегменты кода более легкими для чтения, а другие - более сложными. Затем определите показатели качества или атрибуты чистого кода, к которым вы, как команда, хотели бы стремиться. В конце концов, даже работая над «зелеными» проектами, мы тратим более 90% времени на чтение, и как только код написан (скажем, через 10–15 минут), он уходит в техобслуживание, где читаемость становится еще важнее.
Так что для вашего конкретного примера здесь я бы использовал аргумент, что меньше кода всегда легче прочитать, чем больше кода. Функция, которая имеет 3 параметра, для мозга труднее обрабатывать, чем функция, которая не имеет ни одного параметра или 1 параметра. Если есть другое имя переменной, мозг должен отслеживать еще одну вещь при чтении кода. Итак, чтобы запомнить «int m_value», а затем «int localValue» и помнить, что одно действительно означает, что другое всегда дороже для вашего мозга, чем просто работа с «m_value».
Для большего количества боеприпасов и идей я бы порекомендовал взять копию Чистого кода дяди Боба .
источник
Я могу придумать оправдание для передачи поля члена как параметра в (приватном) методе: он делает явным то, от чего зависит ваш метод.
Как вы говорите, все поля-члены являются неявными параметрами вашего метода, потому что весь объект является. Однако нужен ли полный объект для вычисления результата? Если
SomeMethod
это внутренний метод, который зависит только от того_someField
, не чище ли сделать эту зависимость явной? Фактически, если сделать эту зависимость явной, это также может указывать на то, что вы на самом деле можете рефакторировать этот кусок кода из своего класса! (Обратите внимание, я предполагаю, что мы говорим здесь не о методах получения или установки, а о коде, который фактически что-то вычисляет)Я бы не стал приводить такой же аргумент для открытых методов, потому что вызывающая сторона не знает и не заботится о том, какая часть объекта важна для вычисления результата ...
источник
_someField
это единственный параметр, необходимый для вычисления результата, и мы просто сделали его явным. (Примечание: это важно. Мы не добавили зависимость, мы только сделали ее явной!)this
(илиself
), но это делается явно самим вызовомobj.method(x)
). Другие неявные зависимости - это состояние объекта; это обычно затрудняет понимание кода. По возможности - и в пределах разумного - делайте зависимости явными и функциональными. В случае частных методов, если возможно, явно передавайте каждый параметр, который им нужен. И да, это помогает изменить их.Я вижу одну вескую причину для передачи переменных-членов в качестве аргументов функции частным методам - чистота функции. Переменные-члены фактически являются глобальным состоянием с точки зрения, более того, изменяемым глобальным состоянием, если указанный член изменяется во время выполнения метода. Заменив ссылки на переменные-члены параметрами метода, мы можем эффективно сделать функцию чистой. Чистые функции не зависят от внешнего состояния и не имеют побочных эффектов, всегда возвращая одинаковые результаты при одинаковом наборе входных параметров, что облегчает тестирование и дальнейшее обслуживание.
Конечно, это не просто и не практично, чтобы все ваши методы были чистыми методами на языке ООП. Но я полагаю, что с точки зрения ясности кода вы значительно выиграете, если будете иметь методы, которые обрабатывают сложную логику, и использовать неизменяемые переменные, сохраняя при этом нечистую «глобальную» обработку состояний в отдельных выделенных методах.
Передача переменных-членов в открытую функцию того же объекта при внешнем вызове функции, однако, на мой взгляд, будет основным запахом кода.
источник
Если функция вызывается несколько раз, иногда передавая эту переменную-член, а иногда передавая что-то еще, это нормально. Например, я бы не стал считать это плохим:
где
newStartDate
некоторая локальная переменная иm_StartDate
является переменной-членом.Однако, если функция вызывается только с переданной ей переменной-членом, это странно. Функции-члены постоянно работают с переменными-членами. Они могут делать это (в зависимости от языка, на котором вы работаете), чтобы получить копию переменной-члена - если это так, и вы не видите этого, код может быть лучше, если он сделал весь процесс явным.
источник
Что никто не затронул, так это то, что SomeMethod защищен виртуально. Это означает, что производный класс может использовать его и повторно реализовать его функциональность. Производный класс не будет иметь доступа к закрытой переменной и, следовательно, не сможет обеспечить пользовательскую реализацию SomeMethod, которая зависит от закрытой переменной. Вместо того, чтобы брать зависимость от закрытой переменной, объявление требует, чтобы вызывающий передавал ее.
источник
Фреймворки GUI обычно имеют своего рода класс «View», который представляет вещи, нарисованные на экране, и этот класс обычно предоставляет метод, подобный тому,
invalidateRect(Rect r)
чтобы пометить часть его области рисования как нуждающуюся в перерисовке. Клиенты могут вызывать этот метод для запроса обновления части представления. Но представление может также вызвать свой собственный метод, например:вызвать перерисовку всей области. Например, это может быть сделано при первом добавлении в иерархию представлений.
В этом нет ничего плохого - рамка представления является допустимым прямоугольником, и само представление знает, что оно хочет перерисовать себя. Класс View может предоставлять отдельный метод, который не принимает параметров и использует вместо этого фрейм представления:
Но зачем добавлять специальный метод именно для этого, когда вы можете использовать более общий
invalidateRect()
? Или, если вы решили предоставитьinvalidateFrame()
, вы, скорее всего, реализовали бы его с точки зрения более общегоinvalidateRect()
:Вы должны передать переменные экземпляра в качестве параметров своему собственному методу, если метод не работает конкретно с этой переменной экземпляра. В приведенном выше примере кадр представления - это просто еще один прямоугольник в том, что касается
invalidateRect()
метода.источник
Это имеет смысл, если метод является вспомогательным методом. Скажем, например, вам нужно получить уникальное короткое имя из нескольких строк свободного текста.
Вы не хотели бы кодировать отдельную реализацию для каждой строки, вместо этого имеет смысл передать строку в общий метод.
Однако, если метод всегда работает на одном элементе, кажется немного глупым передавать его в качестве параметра.
источник
Основная причина наличия переменной-члена в классе - это возможность установить ее в одном месте и получить доступ к ее значению для любого другого метода в классе. В общем, поэтому вы ожидаете, что нет необходимости передавать переменную-член в метод класса.
Однако я могу вспомнить пару причин, по которым вам может понадобиться передать переменную-член в другой метод класса. Во-первых, если вам нужно гарантировать, что значение переменной-члена должно использоваться без изменений при использовании с вызываемым методом, даже если этому методу потребуется изменить фактическое значение переменной-члена в какой-то момент процесса. Вторая причина связана с первой в том смысле, что вы, возможно, захотите гарантировать неизменность значения в рамках цепочки методов - например, при реализации свободного синтаксиса.
Имея все это в виду, я бы не сказал, что код «плох» как таковой, если вы передадите переменную-член одному из методов его класса. Однако я бы предположил, что это, как правило, не идеально, так как это может стимулировать многократное дублирование кода, когда параметр назначается, например, локальной переменной, а дополнительные параметры добавляют «шум» в код, где он не нужен. Если вы являетесь поклонником книги «Чистый код», вы бы знали, что в ней упоминается, что вы должны сохранить минимальное количество параметров метода, и только если у метода нет другого более разумного способа доступа к параметру ,
источник
Некоторые вещи, которые приходят ко мне, когда я думаю об этом:
источник
Вы пропускаете, если параметр ("аргумент") является ссылкой или только для чтения.
Многие разработчики, как правило, напрямую присваивают внутренние поля переменной в методах конструктора. Есть несколько исключений.
Помните, что «сеттеры» могут иметь дополнительные методы, а не просто присваивание, а иногда даже виртуальные методы или вызов виртуальных методов.
Примечание: я предлагаю сохранить внутренние поля свойств как «защищенные».
источник