3

Which structure of writing code is better and why?

if (!file_exists('file.txt')) {
   throw new Exception();
} else {
   //operations
}

or

if (!file_exists('file.txt')) {
   throw new Exception();
} 
//operations

?

I did not find this in PSR documentation.

  • 1
    If the existence of the file is a pre-condition for a lengthy operation, the second option is cleaner. You will have the pre-condition "out of the way" and the reader will not have to think sbout it anymore, it will not be part of the logic that follows. If it were more of a case of doing "one or the other" and either action would be equally short, the first option would be better, stressing the fact that the condition determines which of the two possible actions is to be performed. – Martin Maat May 13 '17 at 15:12
11

What your describing in your second code example is a Guard Clause.

Guard clauses exist to help you avoid excessive nesting of conditions, which are hard to maintain and difficult to read:

if ()
{
    throw;
}
else
{
    if()
    {
        return;
    }
    else
    {  
         if()
         {
             log and return;
         }
         else

Writing guard clauses allows you to keep your conditions separated:

if()
{
    throw;
}

if()
{
    return;
}

if()
{
    log and return;
}

// Do actual work

Each guard clause can identify a specific condition or do specific cleanup work prior to exiting the function.

Further Reading
Replace Nested Conditional with Guard Clauses

  • 3
    It's worth noting that this breaks the very useful pattern of a single return used in languages like c to create a single area to clean up resources before the return. The need for that is mitigated in other languages with exceptions which offer different techniques to ensure that resources are always closed: a finally block, using, try-with-resources... – candied_orange May 13 '17 at 14:48
-1

I would apply separations of concern.

Your example shows two responsibilities: checking the preconditions and doing actual work. I would put them in separate methods:

throwIfFileFailsChecks(file);
workWithFileContent(file);

The latter may be split up in even more methods having dedicated "concerns".

  • 1
    @CandiedOrange: Or, y'know, simply if (fileNotFound) throw which is arguably simpler than trying to trivially refactor. – Robert Harvey May 13 '17 at 15:08
  • "cky! Abstractions are good but not when they require comments to understand." Thats true. In Java I can declare the method to throw that exception which would make the comment obsolete. I don't know if PHP has a similar mechanism. Thats why I added the comment. – Timothy Truckle May 13 '17 at 15:09
  • 3
    If your name requires me to look at the declaration your name has already lost to @RobertHarvey 's traditional alternative. The names you put on abstractions should make the code easier to read. Not force me to scroll more. – candied_orange May 13 '17 at 15:11
  • @CandiedOrange "If your name requires me to look at the declaration your name has already lost" The names must express the responsibility of that method. They should not convey implications of their implementation. e.g.: what makes you think the existence of the file is the only reason to fail? – Timothy Truckle May 13 '17 at 15:15
  • 1
    Throwing is not an implementation detail here. It's part of the contract. I can't USE this properly without knowing it throws. You can't hide that it throws from me anymore than you can hide that a function returns something. – candied_orange May 13 '17 at 15:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.