In someone else's code, I often meet the logic of validation of a mandatory condition, designed as follows:

if (обязательное условие) { // много // много десятков // строк // основного // кода } else { // одна строка записи об ошибке в лог // и ещё одна – return или throw или выход, итого две. }

I do not like this design, because in logic, there is clearly a main stream, and optional departures, if some condition fail. And it seems that the deepening of another indent here to anything. I would write such moments like this:

 if ( !обязательное условие) { // одна строка записи об ошибке в лог // и ещё одна – return или throw или выход, итого две. } // дальше много строк основного кода

Is there any logical argument or common practice in addition to my intuitive feeling in favor of one of the options?

  • four
    I overwhelmingly use the second option. And there is no extra indent, and the logic is not "smeared." This case is called the "guard code" (or "guard clause", judging by Wikipedia ). Regarding the generally accepted - here except to collect statistics. - Regent
  • one
    A little related topic . - VladD
  • unless ( unless )! Well, you can still if-not (Clojure) . - D-side

4 answers 4

  • BUT

    + get rid of exclamation marks,

    - we get the extra code nesting,

    - it - not always clear what “non-balance” means.

     if ( balance ){ // pay // and // get // the // food }else{ return("go away") }

  • B

    + return immediately, without unnecessary nesting,

    - exclamation mark

    - it - not always clear what “non-balance” means.

     if ( !balance ){ return ("go away") } // pay // and // get // the // food

  • C

    rename the condition:

    + by the name of the variable it is immediately clear what we mean

    + no extra code nesting,

    - coding negation in the variable name.

     if (notEnoughBalance){ return ("go away"); } // pay // and // get // the // food

  • D

    rename the condition EnoughBalance :

    + by the name of the variable it is immediately clear what we mean

    + no extra code nesting,

    + no coding in variable name.

     if (!EnoughBalance){ return ("go away"); } // pay // and // get // the // food

It's still a matter of preference, programming language and specific project. When I write the code myself in a hurry, I often prefer the B style, but usually I try to write in the C style (until I fall on my own or other people's crutches, so let's add the right D style).

Another plus of the C approach: if you carefully create and name conditions, then, in principle, you will not have to suffer, for example, with the fact that different languages ​​can transform values ​​into bool in different ways.

Style D is actually better than C , because sooner or later someone will write

 if (!notEnoughBalance){// тут у вас ломается голова }

Thank you @Qwertiy , I could not remember a trap that I’ve come across several times in recent years.

In summary: the name of a condition variable or a function that returns a condition should not contain validity conditions, only its meaning:

  • EnoughMoney , AcceptableHeight , TooHeavy valid.

  • NotEnoughMoney , NotAcceptableHeight , NotTooHeavy go forest.

An example from, again, @Qwertiy :

notHasAccess or hasNotAccess can be replaced by isAccessDenied - the same truth coding in the variable name, but semantically more correct, because it contains not only the negation, but also the meaning “access denied”.

  • 3
    Option C is evil. Variable names must not contain negatives. Negation before variable is better than negation in name. You must either honestly set a negative, or rename a variable using other words. - Qwertiy ♦
  • @Qwertiy, yes, I think right now too. Sometimes he ran across semantic rakes with him. On the other hand, it is more convenient when used systematically and correctly. - strangeqargo pm
  • You can still add that you can often just pick up the antonym. For example, notHasAccess or hasNotAccess can be replaced by isAccessDenied . Keep a plus sign. - Qwertiy ♦
  • 2
    But I would not be too categorical about the in / un prefixes, since they still form complete words. They are somewhere on the border. The phrase "height is not inacceptable" is easier to understand than "height is not not acceptable" :) - Qwertiy ♦
  • I do not know whether I am ready to bring this answer to the ideal and whether it is possible at all, but thank you very much. - strangeqargo

The use of if / else / if not depends on the context of the application. If there is a difference of operations, but a single part of the completion of the code, then you need to use the first option.

If, if some condition is not fulfilled, the continuation of the program (method) does not make sense anymore , then it is logical to use the second option.

The advantages of the second method in this context are obvious. When analyzing a code, it is simply easier for you to see and keep in your head the conditions under which code execution will end, therefore you use it. After all, in fact, this is a boundary condition that interrupts the main code.

Suppose there is a task:

Write a controller method in which to issue an error if there is no record, and if successful, change the name field to 1, save to the database and display all information on the user. If the user’s name is Ivan, then just reset the password, but do not change the name.

 $user = Users::find($id); if (!$user) { throw new BadRequestException("User not found"); } if ($user->getName()=="Ivan") { $user->setPassword(null); } else { $user->setName(1); } $user->save(); var_dump($user->toArray());

There is a question of logic, because it is so convenient to read this code and it is immediately clear that if this user does not exist, the execution of the program will be interrupted and there is no sense to read further. Imagine that we use if else, then we get additional nesting if, which complicates the reading of the program and its visibility:

 $user = Users::find($id); if ($user) { if ($user->getName()=="Ivan") { $user->setPassword(null); } else { $user->setName(1); } $user->save(); var_dump($user->toArray()); } else { throw new BadRequestException("User not found"); }

We, as a matter of fact, here have carried out the basic code of the program in a condition that is not quite right. After all, the entire controller method may depend on this $user , and you will have to render all the code in an if.

It seems to me that you need to avoid unnecessary nesting , if there is a reasonable solution method.

    In my opinion, in the case of “asymmetric” branches, especially when a short branch completes execution, it makes no sense to pull it to a point after a long branch. This worsens the clarity of the code, because otherwise the reader is forced to keep in mind the presence of an alternative option, reading the entire code of the positive if branch.

    Usually, in such checks, the input data is first checked, or the “special” case is cut off (the input parameter is null , the task is already running, the application is already terminating, etc.), and the rest of the code works with the “typical” case.

    Alternatively, when a short checkout and exit first occurs, the reader can immediately after reading the conditions and actions to exit the procedure switch to the “normal” case and not keep in mind the potentially difficult pending task (“you will need to somehow handle the case, when the condition is not satisfied ").

    It is better not to bother the reader of the code with unnecessary constructions; it is not easy to understand the code (no difference, yours or someone else's) without it.

      In most cases, if the exit is more convenient, because it does not send all the code to an extra level of nesting. On the other hand, if we are already talking about some kind of nested structure, then I prefer a plus level than an extra code next to it. And let them be there at least a pack - the nesting chain is easier to read than a mixture of linear constructions with nesting.

      For example, I will arrange ifs like this:

       function sum(a) { var res = 0; if (!a) { return 0; } for (var r of a) { if (r && r.length) { for (var x of r) { if (x > 0) { res += x; } } } } return res; } sum([[1,2,4,-4,-2,-1],888,[312]])

      but not so (although if (a) somehow fits organically into the structure - specifically in this case it was possible to wrap the cycles in it, but in most cases this is not so):

       function sum(a) { var res = 0; if (a) { for (var r of a) { if (!r || !r.length) { continue; } for (var x of r) { if (x <= 0) { continue; } res += x; } } } return res; } sum([[1,2,4,-4,-2,-1],888,[312]])

      PS: There is another topic about braces .