Кодовая база, над которой я работаю, часто использует переменные экземпляра для обмена данными между различными тривиальными методами. Первоначальный разработчик непреклонен, что он придерживается лучших практик, изложенных в книге « Чистый код » дядюшки Боба / Роберта Мартина: «Первое правило функций заключается в том, что они должны быть маленькими». и «Идеальное число аргументов для функции - ноль (нильадик). (...) Аргументы - это сложно. Они требуют много концептуальной силы».
Пример:
public class SomeBusinessProcess {
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;
private byte[] encodedData;
private EncryptionInfo encryptionInfo;
private EncryptedObject payloadOfResponse;
private URI destinationURI;
public EncryptedResponse process(EncryptedRequest encryptedRequest) {
checkNotNull(encryptedRequest);
getEncodedData(encryptedRequest);
getEncryptionInfo();
getDestinationURI();
passRequestToServiceClient();
return cryptoService.encryptResponse(payloadOfResponse);
}
private void getEncodedData(EncryptedRequest encryptedRequest) {
encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
}
private void getEncryptionInfo() {
encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
}
private void getDestinationURI() {
destinationURI = router.getDestination().getUri();
}
private void passRequestToServiceClient() {
payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);
}
}
Я бы преобразовал это в следующее, используя локальные переменные:
public class SomeBusinessProcess {
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;
public EncryptedResponse process(EncryptedRequest encryptedRequest) {
checkNotNull(encryptedRequest);
byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
URI destinationURI = router.getDestination().getUri();
EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
encryptionInfo);
return cryptoService.encryptResponse(payloadOfResponse);
}
}
Это короче, оно устраняет неявную связь данных между различными тривиальными методами и ограничивает области видимости до минимума. Тем не менее, несмотря на эти преимущества, я все еще не могу убедить первоначального разработчика в том, что этот рефакторинг оправдан, так как, по-видимому, он противоречит практике дяди Боба, упомянутой выше.
Отсюда мои вопросы: какова цель, научное обоснование предпочтения локальных переменных переменным экземпляра? Кажется, я не могу понять, как это сделать. Моя интуиция говорит мне, что скрытые связи плохие и что узкая область лучше, чем широкая. Но какая наука поддерживает это?
И наоборот, есть ли недостатки этого рефакторинга, которые я, возможно, упустил из виду?
источник
Ответы:
Сфера - это не двоичное состояние, а градиент. Вы можете ранжировать их от самых больших до самых маленьких:
Изменить: то, что я называю «область видимости класса», это то, что вы подразумеваете под «переменной экземпляра». Насколько мне известно, это синонимы, но я разработчик C #, а не разработчик Java. Ради краткости я объединил всю статику в глобальную категорию, поскольку статика не является темой вопроса.
Чем меньше сфера, тем лучше. Обоснование состоит в том, что переменные должны жить в наименьшем возможном объеме . Есть много преимуществ для этого:
Name
свойство, вы не принуждали префикс их , какFooName
,BarName
... Таким образом , сохраняя ваши имена переменных , как чистый и лаконичный , насколько это возможно.passRequestToServiceClient()
в начало метода, и он все равно будет компилироваться. С локальными пользователями вы могли бы совершить эту ошибку, только если передали неинициализированную переменную, что, как мы надеемся, достаточно очевидно, что вы на самом деле этого не делаете.Проблема здесь в том, что ваш аргумент для локальных переменных действителен, но вы также внесли дополнительные изменения, которые не являются правильными и приводят к тому, что предлагаемое исправление не проходит тест на запах.
Хотя я понимаю ваше предложение «без переменной класса» и в этом есть смысл, вы на самом деле также удалили сами методы, и это совершенно другая игра. Методы должны были остаться, и вместо этого вы должны были изменить их, чтобы они возвращали свое значение, а не сохраняли его в переменной класса:
Я согласен с тем, что вы сделали в
process
методе, но вы должны были вызывать закрытые субметоды, а не выполнять их тела напрямую.Вы хотели бы этот дополнительный уровень абстракции, особенно когда вы сталкиваетесь с методами, которые необходимо использовать несколько раз. Даже если вы в настоящее время не используете свои методы повторно , хорошая практика заключается в том, чтобы уже создавать подметоды там, где это уместно, даже если они помогают улучшить читаемость кода.
Независимо от аргумента локальной переменной, я сразу заметил, что предложенное вами исправление значительно менее читабельно, чем оригинал. Я признаю, что бессмысленное использование переменных класса также снижает читабельность кода, но не на первый взгляд, по сравнению с тем, как вы сложили всю логику в одном (теперь многословном) методе.
источник
Исходный код использует переменные-члены, такие как аргументы. Когда он говорит минимизировать количество аргументов, он на самом деле имеет в виду минимизацию количества данных, которое требуется методам для функционирования. Помещение этих данных в переменные-члены ничего не улучшает.
источник
process.Start();
илиmyString.ToLowerCase()
не должны казаться слишком странными (и, действительно, их легче понять).this
. Можно даже утверждать, что этот аргумент приводится явно - до точки.Другие ответы уже прекрасно объяснили преимущества локальных переменных, так что все, что осталось, это часть вашего вопроса:
Это должно быть легко. Просто укажите ему следующую цитату в Чистом коде дяди Боба:
То есть дядя Боб не просто говорит, что функция должна принимать несколько аргументов, он также говорит, что функции должны по возможности избегать взаимодействия с нелокальным состоянием.
источник
«Это противоречит тому, что думает чей-то дядя», НИКОГДА не хороший аргумент. НИКОГДА. Не бери мудрости у дяди, думай сам.
Тем не менее, переменные экземпляра должны использоваться для хранения информации, которая фактически должна храниться постоянно или полупостоянно. Информация здесь не. Это очень просто жить без переменных экземпляра, чтобы они могли идти.
Тест: написать комментарий к документации для каждой переменной экземпляра. Вы можете написать что-нибудь, что не является абсолютно бессмысленным? И написать комментарий к документации для четырех участников. Они одинаково бессмысленны.
Хуже всего, предположите способ расшифровки изменений, потому что вы используете другой криптосервис. Вместо того, чтобы менять четыре строки кода, вы должны заменить четыре переменные экземпляра на разные, четыре метода получения на разные и изменить четыре строки кода.
Но, конечно, первая версия предпочтительнее, если вам платит строка кода. 31 строка вместо 11 строк. В три раза больше строк для записи и для сохранения навсегда, для чтения при отладке чего-либо, для адаптации при необходимости изменений, для дублирования при поддержке второго cryptoService.
(Пропустил важный момент: использование локальных переменных заставляет вас делать вызовы в правильном порядке).
источник
Переменные экземпляра предназначены для представления свойств их главного объекта, а не для представления свойств, специфичных для потоков вычислений, более узко ограниченных, чем сам объект. Некоторые из причин проведения такого различия, которые, по-видимому, еще не были рассмотрены, связаны с параллелизмом и повторным входом. Если методы обмениваются данными, устанавливая значения переменных экземпляра, то два параллельных потока могут легко засорять значения друг друга для переменных этих экземпляров, приводя к периодическим, трудно обнаруживаемым ошибкам.
Даже один поток может столкнуться с проблемами в этом направлении, потому что существует высокий риск того, что шаблон обмена данными, основанный на переменных экземпляра, делает методы не реентерабельными. Точно так же, если одни и те же переменные используются для передачи данных между различными парами методов, существует риск того, что один поток, выполняющий даже нерекурсивную цепочку вызовов методов, столкнется с ошибками, возникающими вокруг неожиданных модификаций задействованных переменных экземпляра.
Чтобы получить достоверно правильные результаты в таком сценарии, вам нужно либо использовать отдельные переменные для связи между каждой парой методов, где один вызывает другой, либо чтобы каждая реализация метода учитывала все детали реализации всех других методы, которые он вызывает, прямо или косвенно. Это хрупко и плохо масштабируется.
источник
public EncryptedResponse process(EncryptedRequest encryptedRequest)
не синхронизирован, и параллельные вызовы вполне могут привести к засорению значений переменных экземпляра. Это хороший момент для воспитания.Обсуждая справедливо
process(...)
, пример ваших коллег гораздо более разборчив в смысле бизнес-логики. И наоборот, ваш встречный пример требует не только беглого взгляда, чтобы извлечь какой-либо смысл.При этом чистый код является и разборчивым, и хорошего качества - выталкивание локального состояния в более глобальное пространство - это просто сборка высокого уровня, поэтому ноль для качества.
Это представление, которое устраняет необходимость в переменных в любой области видимости. Да, компилятор сгенерирует их, но важной частью является то, что он контролирует, чтобы код был эффективным. Хотя также является относительно разборчивым.
Просто пункт о наименовании. Вы хотите самое короткое имя, которое имеет смысл и расширяет доступную информацию. то есть. destinationURI, URI уже известен по сигнатуре типа.
источник
Я бы просто удалил эти переменные и приватные методы. Вот мой рефакторинг:
Для частного метода, например
router.getDestination().getUri()
, более понятный и более читаемый, чемgetDestinationURI()
. Я бы даже повторил это, если бы я использовал одну и ту же строку дважды в одном классе. Если взглянуть на это иначе, если есть необходимостьgetDestinationURI()
, то он, вероятно, принадлежит другому классу, а неSomeBusinessProcess
классу.Для переменных и свойств общая потребность в них состоит в том, чтобы хранить значения, которые будут использоваться позже во времени. Если у класса нет открытого интерфейса для свойств, они, вероятно, не должны быть свойствами. Худший вид использования свойств класса, вероятно, для передачи значений между закрытыми методами посредством побочных эффектов.
В любом случае, классу нужно только сделать,
process()
и тогда объект будет отброшен, нет необходимости сохранять какое-либо состояние в памяти. Дальнейший потенциал рефакторинга будет заключаться в том, чтобы вывести CryptoService из этого класса.Основываясь на комментариях, я хочу добавить, что этот ответ основан на реальной мировой практике. Действительно, в обзоре кода первое, что я бы выбрал, - это рефакторинг класса и удаление работы шифрования / дешифрования. Как только это будет сделано, я бы спросил, нужны ли методы и переменные, правильно ли они названы и так далее. Окончательный код, вероятно, будет ближе к этому:
С приведенным выше кодом, я не думаю, что это требует дальнейшего рефакторинга. Как и в случае с правилами, я думаю, что требуется опыт, чтобы знать, когда и когда их не применять. Правила - это не теории, доказавшие свою эффективность во всех ситуациях.
С другой стороны, проверка кода оказывает реальное влияние на то, как долго может пройти кусок кода. Моя хитрость заключается в том, чтобы иметь меньше кода и сделать его легким для понимания. Имя переменной может быть предметом обсуждения, если я смогу удалить его, рецензентам даже не нужно будет об этом думать.
источник
Ответ Флэтера достаточно хорошо охватывает вопросы определения объема, но я думаю, что здесь есть и другая проблема.
Обратите внимание, что есть разница между функцией, которая обрабатывает данные, и функцией, которая просто обращается к данным .
Первый выполняет реальную бизнес-логику, тогда как второй экономит набор текста и, возможно, повышает безопасность, добавляя более простой и более повторно используемый интерфейс.
В этом случае кажется, что функции доступа к данным не сохраняют набор текста и нигде не используются повторно (или могут возникнуть другие проблемы при их удалении). Так что эти функции просто не должны существовать.
Сохраняя только бизнес - логику названных функций, мы получаем лучшее из обоих миров (где - то между ответом Flater в и ответ imel96 в ):
источник
Первая и самая важная вещь: дядя Боб, кажется, иногда похож на проповедника, но утверждает, что есть исключения из его правил.
Вся идея Чистого кода состоит в том, чтобы улучшить читаемость и избежать ошибок. Есть несколько правил, которые нарушают друг друга.
Его аргумент в отношении функций заключается в том, что функции niladic являются лучшими, однако допустимо до трех параметров. Я лично думаю, что 4 тоже в порядке.
Когда используются переменные экземпляра, они должны составлять связный класс. Это означает, что переменные должны использоваться во многих, если не во всех нестатических методах.
Переменные, которые не используются во многих местах класса, должны быть перемещены.
Я не считаю ни оригинальную, ни реорганизованную версию оптимальными, и @Flater уже очень хорошо сказал, что можно сделать с возвращаемыми значениями. Это улучшает читаемость и уменьшает количество ошибок при использовании возвращаемых значений.
источник
Локальные переменные уменьшают область действия и, следовательно, ограничивают способы использования переменных и, следовательно, помогают предотвратить определенные классы ошибок и улучшают читабельность.
Переменная экземпляра уменьшает способы вызова функции, что также помогает уменьшить определенные классы ошибок и улучшает удобочитаемость.
Сказать, что одно правильно, а другое неправильно, вполне может быть верным выводом в любом конкретном случае, но в качестве общего совета ...
TL; DR: Я думаю, что причина того, что ты слишком сильно пахнешь, в том, что слишком много рвения.
источник
Несмотря на то, что методы, начинающиеся с get ..., не должны возвращать void, разделение уровней абстракций внутри методов дано в первом решении. Хотя второе решение более ограничено, все еще сложнее рассуждать о том, что происходит в методе. Назначения локальных переменных здесь не нужны. Я бы сохранил имена методов и изменил бы код на что-то вроде этого:
источник
Обе вещи делают то же самое, и различия в производительности незаметны, поэтому я не думаю, что есть научный аргумент. Это сводится к субъективным предпочтениям тогда.
И мне тоже нравится ваш путь лучше, чем ваш коллега. Почему? Потому что я думаю, что легче читать и понимать, несмотря на то, что говорит автор какой-то книги.
Оба пути выполняют одно и то же, но его путь более распространен. Чтобы прочитать этот код, вам нужно переключаться между несколькими функциями и переменными-членами. Это не все сжато в одном месте, вам нужно запомнить все это в своей голове, чтобы понять это. Это намного большая когнитивная нагрузка.
Напротив, ваш подход упаковывает все гораздо плотнее, но не так, чтобы сделать его непроницаемым. Вы просто читаете это строка за строкой, и вам не нужно запоминать так много, чтобы понять это.
Однако, если он привык к такому кодированию, я могу представить, что для него это может быть наоборот.
источник