Чи є погана практика використання умов, що мають функції, що змінюють стан програми?

Назва може бути трохи розпливчастою, так що дозвольте мені пояснити. Припустимо, у нас є функція, яка щось робить (змінює стан програми), наприклад функцію, яка створює файл. Ця функція повертає True, якщо файл створено, і False, якщо файл не створено.

Тепер ми хочемо використовувати цю функцію в умовному, наприклад:

if (createFile() == false)
   //log: we cannot create file

І ми можемо зробити це наступним чином:

boolean fileCreated = createFile()
if (fileCreated == false)
   //log: we cannot create file

Питання в тому, чи є перший випадок гіршим, ніж другий, з точки зору читабельності та ясності, і який з них рекомендується використовувати?

Моя аргументація полягає в тому, що якщо хтось читає код, можливо, він не знайомий з внутрішніми функціями функції, то в першому випадку він може припустити, що функція createFile() не змінює стан (оскільки часто ці функції є предикатними функціями)?

4
Я не думаю, що з цим виникає проблема. Просто не називайте методи з побічними ефектами, як "isFileReady ()", що передбачає незмінність.
додано Автор Justin Poliey, джерело
Я не розумію, чому друга версія передає ідею, що createFile() змінить стан, але перша версія цього не зробить.
додано Автор David Arno, джерело
@yegodm, якщо це дійсно типово, тоді folk зазвичай пише поганий код. Виключіть винятки в обставинах, за винятком того, що золотого шляху через код не відбулося.
додано Автор David Arno, джерело
Я вважаю, що читабельність в очах читача, оскільки єдина річ, яка виділяється для мене, це те, що жахливий == false , а не ! Крім цього, перша версія виглядає краще для мене, але це дійсно просто думка.
додано Автор David Arno, джерело
@yegodm Я абсолютно з вами згоден. Я особисто використовую пакет NIO. Однак це не є предметом обговорення. Я запитую загалом, і це лише приклад того, що це відбувається в реальному житті. Програмісти, мабуть, повинні були використовувати це замість Guava або NIO, перш ніж вони були створені.
додано Автор Krzysztof Stanisławek, джерело
@yegodm Я згоден, що це буде поганий дизайн функції. Але конкретним прикладом є функція File.mkdirs() в Java, яку ми не можемо змінити, і пакет .io є поганим, я з цим погоджуюсь. Оскільки хтось може не мати доступу до .nio, це викликало у мене цікавість з цим.
додано Автор Krzysztof Stanisławek, джерело
@DavidArno Я використав == false , тому що я намагався написати приклади мовним методом і подумав, що це ще більше підкреслить контекст. Я також погоджуюся, що ! ... є більш зручною для читання, але це інша дискусія. Що стосується вашого другого коментаря, мої міркування пояснюються у відповіді: назва функції вказує на те, що він змінює стан, але використання його в умовному вигляді зводить нанівець, що, оскільки такі функції, як правило, є предикатами (і, таким чином, можуть створити певні припущення).
додано Автор Krzysztof Stanisławek, джерело
Що я намагаюся сказати - я б не рекомендував так чи інакше, і, якщо б це я робив огляд коду для товариша по команді, запропонував би два варіанти: (а) перейменувати функцію на "tryCreateFile (), або (b) перенести з інша функція кидає виняток.
додано Автор snetch, джерело
@leonz Якщо це поганий код, ми можемо зробити це краще. Наприклад, Google Guava певною мірою намагається виправити File.mkdirs() ( javadoc.scijava.org/Guava/com/google/common/io/… ) кидаючи IOException/code> якщо каталог не створено.
додано Автор snetch, джерело
@DavidArno Ось чому я ставлю , але не робить цього . Якщо результат є одним з очікуваних, то функція може бути названа `tryCreateFile() '.
додано Автор snetch, джерело
Як правило, якщо функція повинна змінювати стан, але не вдається зробити це, то виникає виняток. Тоді ваш createFile() буде просто void , а "if" взагалі не буде потрібно.
додано Автор snetch, джерело

5 Відповіді

Для мене основний запах коду полягає в тому, що метод createFile() повертає булев, що вказує на успіх. Це нагадує мені про стилі програмування 1970-х років.

Метод з ім'ям createFile() означає контракт, який він створює файл, і якщо він не може, контракт не виконується, і метод повинен кинути виняток. За допомогою булево-повертаючої версії, трохи недбалий користувач, звикший до поточної практики програмування, побачить це ім'я і викликатиме метод, ігноруючи значення булевого повернення, і припустимо, що файл створено.

Якщо ви хочете, щоб контракт дозволив не створювати файл за певних обставин, ви повинні вказати, що в назві, наприклад, як createFileIfPossible() .

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

10
додано
Краще tryCreateFile() . Він коротший і не менш описовий.
додано Автор Shizam, джерело
+1 для виключення виключення, якщо створення не вдалося. Історія повідомляє нам, що програмісти загальновідомо нерозумні щодо перевірки кодів повернення, що дозволяє розповсюджувати помилку, можливо, цікавими способами, і може зробити її важкою. Винятки зазвичай забезпечують негайне відстеження.
додано Автор Mark McKinstry, джерело

Це трохи виклику судження, але я б сказав, що якщо вам потрібно зателефонувати createFile() на початку методу, це трохи краще зберігати результат у змінній. Це відбувається тому, що якщо хтось оновлює код пізніше і повинен перевірити стан fileCreated , звичайно копіювати умову з існуючого блоку коду. Якщо ви використовуєте метод безпосередньо, редактор повинен ввести змінну та оновити існуючу умову. Не дійсно кінець світу таким чином я би не приїхав повішений на це. Якщо остання операція return createFile() , я б не ввів змінну.

Дійсно, хоча краще не перевіряти таку саму умову більше одного разу в методі, таким чином уникаючи того, що цілком є ​​оптимальним. Це зробило б вищезгаданим безглуздо. Так що це не дуже різана і висушена річ. Це залежить від рівня кваліфікації команди.

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

4
додано
Я очікую, що професійний інженер-програміст не буде сліпо копіювати та вставляти умову if і рухатися далі. Це погана причина, щоб зробити код, який вже зрозуміло більш детальний.
додано Автор Andy, джерело
Ви також повинні думати про ідіоми. Багато поширені схеми зчитування потоку в C/C ++ змінюють стан в стані циклу while.
додано Автор Frank Hileman, джерело
@FrankHileman Я думаю, що саме це підштовхнуло мене до цієї відповіді. Якщо ви називаєте щось таким чином, що схоже на таку ідіому, є більше шансів, що хтось буде використовувати його таким чином. Призначення результату до (останньої) змінної - це ідіома, яка припускає, що це те, що потрібно робити один раз і лише один раз.
додано Автор JimmyJames, джерело
@Alexander Як я вже сказав, це не дуже скоротити і висушити. Проблема в тому, що вам потрібно знати, щоб рефактор. IDE не збирається розповідати вам, що ви повинні робити це, а не копіювати і вставляти код. Наприкінці дня, це людина, яка скопіювала і вставила, хто б винен. Я бачив, що така помилка викликає помилки. В ідеалі це було б випробувано. Я нікого не звинував би, що не хочуть додаткової лінії, і я схильний робити це по-своєму. Можливо, якщо назвати її більш ніж один раз, це було дуже погано, я міг би додати це як застереження.
додано Автор JimmyJames, джерело
"Не зовсім кінець світу, тому я б не зависав на ньому". Мені не подобається робити більше-багато-ніж потрібний код зараз, просто щоб оптимізувати для випадковості я, можливо, доведеться змінити його в майбутньому. Зміна полягає лише в рефакторінгу IDE, немає сенсу намагатися "уникнути" необхідності цього, це тривіально.
додано Автор Alexander, джерело
Так, я згоден, ви повинні пам'ятати про sideeffects при копіюванні викликів функцій. Хоча я б підійшов до цього по-іншому, замість того, щоб createFile повернути Опціонально <�Файл> , призначивши його, якщо необхідно, інакше просто зробивши if (createFile (). )) . Theres трохи більше відбуваються там, котрий я думаю би зробив люди беруть секунду переглядати copy/paste, у подібний спосіб того, як невизначеність навколо перехресть без знаку змушує людей їздити більш обережно.
додано Автор Alexander, джерело

Використання значення return з функції безпосередньо, замість того, щоб зберігати її у змінну, цілком нормально, якщо вам потрібен лише один раз. Насправді, не вводячи зайві артефакти, це гарна ідея, оскільки, таким чином, ви не можете втратити їх, ані погано назвати.

Не дуже добре, що функції називають. Я б не очікував, що він поверне bool , але File або будь-що інше, і викине виняток у разі невдачі. У мовах, що підтримують, він, ймовірно, повинен бути ctor Крім того, перейменування tryOpenFile() є прийнятним.
Ще однією дивовижною ситуацією є те, що не існує аргументу для імені файлу.

3
додано

Ігноруйте фактичні виклики на секунду, тому що вони є деталізацією реалізації і повинні бути оброблені такими. Перше завдання: спробував створити файл і встановив логічне "fileCreated" на false.

Ваше друге завдання: якщо файл не був створений, зареєструйте факт.

Тепер напишіть код для другого завдання. Ви маєте бути в змозі зробити це без написання коду для першого завдання. Як наслідок, ви не можете мати такий вигляд, як "if (createFile() == false). Ваш перший приклад коду прийшов до створення файлу і тесту. , але в більших випадках це стане потворним.

0
додано

Код повинен бути легко читається. Якщо ви змусите розробника спробувати знайти те, що дійсно відбувається у вашому коді, щось не так.

Обидва випадки для мене читаються, але це не означає, що вони не можуть бути покращені. Я розумію, що повернення "створення файлу" не так очевидно з точки зору сенсу. Моя ідея - створити новий метод (замість змінної), щоб зробити читання коду більш природним.

Отже, ви можете трохи покращити:

if (!isFileCreated(createFile())
   //log: we cannot create file
0
додано
З урахуванням звичайних умовностей, я б пішов далі і написав це як щось подібне: if (! IsFileCreated (handle = createFile ()) log_error_and_quit (). відкрийте кілька файлів і маніпулюйте ними, просто жонглюючи ручками та +1 для зручності читання.
додано Автор Mark McKinstry, джерело