Немного о правильности написания кода. Какой вариант выбрать?

Question

[Сергей Семенко]

Допустим, имеется такой обработчик нажатий:

@Override
    public void onButtonClick() {
        if (isRequested()) {
            if (rulesField.isChecked()) {
                register(numberField.getText().toString(), codeField.getText().toString());
            } else {
                showMessage(R.string.error_rules);
            }
        } else {
            requestRegister(numberField.getText().toString());
        }
    }

Вроде ведь ничего плохого, но знакомые не особо такое воспринимают. В чём же здесь проблема?

Предлагали такой вариант:

@Override
    public void onButtonClick() {
        if (isRequested() && rulesField.isChecked()) {
            register(numberField.getText().toString(), codeField.getText().toString());
            return;
        }
        if (isRequested() && !rulesField.isChecked()) {
            showMessage(R.string.error_rules);
            return;
        }
        if (!isRequested()) {
            requestRegister(numberField.getText().toString());
        }
    }

Как по вашему мнению лучше писать? Или вообще без разницы?

P.S. #1: Привёл пример небольшой конструкции, условий может быть больше.
P.S. #2: Вопрос может и глупый, но меня он почему-то беспокоит :)

Answer 1

[Artem Gapchenko]

Много уровней вложенности плохо читаются. Попробуйте добавить внутрь условия

if (rulesField.isChecked()) {
    register(numberField.getText().toString(), codeField.getText().toString());
}

Ещё парочку вложенных if-else, чтобы получилось вот такое:

@Override
    public void onButtonClick() {
        if (isRequested()) {
            if (rulesField.isChecked()) {
                    if (canProceed()) {
                         register(numberField.getText().toString(), codeField.getText().toString());
                    } else if (isExtraRequestRequired()) {
                        doSomething();
                     } else if (oneMoreCondition()) {
                         doSmomethingOnExtraCondition();
                     } else {
                          Log.e("Unexpected condition");
                     }
            } else {
                showMessage(R.string.error_rules);
            }
        } else {
            requestRegister(numberField.getText().toString());
        }
    }

и вы поймёте, что удерживать в голове ту ветку, которая вам интересна (сначала if, потом вложенный в него else, потом второй if-else из вложенных в else первого if'а) становится решительно невозможно. Поэтому и стараются так структурировать код, чтобы он был "плоским", то есть без вложенности. Это, конечно идеал, и иногда труднодостижимый, но стремиться к нему стоит - человек, который после вас будет читать ваш листинг (а чаще всего это будете вы сами недели через две после того, как вы его написали, и успешно забыли всю структуру), скажет вам спасибо.

Comments

[Artem Gapchenko]

Макконнелл в "Совершенном Коде" пишет, что, цитирую "немногие люди способны понять более трех уровней вложенности if", ссылаясь на работы Ноама Хомски. Холиварный, конечно, вопрос, но я предпочитаю писать код понятный большинству, а не только мне.

[Artem Gapchenko]

С короткими методами и рефакторингом абсолютно согласен.

Answer 2

[Tiberal]

то, что предложили хуже

isRequested() = false -> 3 раза дернется if

в вашем случае один раз

ну и второй вариант плохочитаем

если совсем много вложений switch в помощь

Comments

[Artem Gapchenko]

Как вам switch поможет в случае вложенных условных конструкций? case-ы всё равно будут плоско располагаться.

[Tiberal]

смотря какое условие, в большинстве случаев можно здорово упростить себе жизнь при помощи switch. А если писать еще на каком-нибудь котлине где switch здорово прокачали так вообще. Да и результат будет читабельнее.

Answer 3

[anton_lazarev]

Первый вариант, рано или поздно, может превратиться в:

Comments

[Сергей Семенко]

Ну не, это перебор, до такого у меня точно не дойдёт.

Answer 4

[AtomKrieg]

Вариант для отвeта Ivan Sokolov

if (!isRequested()) {
 requestRegister(numberField.getText().toString());
 return;
}

if (rulesField.isChecked()) {
    register(numberField.getText().toString(), codeField.getText().toString());
} else {
    showMessage(R.string.error_rules);
}