CODEWITHSUNDEEP
functionrefactoring
javier_domenech
javier_domenech

Reputation: 6263

function returning value or integer status code refactor

I'm dealing with several functions with some code smell:

public function example($a, $b, $c) {
    $something = doSomething($a);
    $more = doMoreStuff($b,$c);
    $evenMore = doEvenMoreStuff($a,$c);

    if(!$something) {
        //error code because blablabla
        return 1;
    }

    if(!$something and $more == "whatever") {
        //another different error because blebleble
        return 2;
    }

    if(!$more) {
        //this means another error because bliblibli
        return 3;
    }

    if(!$evenMore) {
        //yep, error, returning code error 4
        return 4;
    }

    //etc...

    //if no errors
    return $something + $more + $evenMore;
}

How should I deal with these error codes? I'm thinking in creating a class with const values like so:

class ExampleError {

    const BLABLABLA = 1;
    const BLEBLEBLE = 2;
    const BLIBLIBLI = 3;
    const BLOBLOBLO = 4;
}

And then refactor the function changing lines like

return 1;;

to

return ExampleError::BLABLABLA;

trying to make it more readable. Any better aproach?

Upvotes: 0

Views: 112

Answers (1)

Neville Kuyt
Neville Kuyt

Reputation: 29639

It depends on how the rest of your application is written, and on whether what you call "errors" are indeed errors, or predictable, legitimate edge cases.

The first code smell is "magic numbers"; yes, replace them with symbolic constants.

The second code smell is that your return signature is not "clean" - you either return an error code, or a result set. In doing so, you require the calling code to understand the semantics of your code - it needs to know that BLABLABLA is an unrecoverable error connecting to the database, and BLEBLEBLE means you've violated some constraint somewhere.

Your question suggests that your code may encounter errors in generating the results; the "cleanest" way is to use exceptions to indicate errors. Exceptions have built-in semantics or indicating what went wrong, can be "wrapped" and can propagate up the call stack. So, if BLABLABLA means you cannot connect to the database, the method calling your function can pass that back up to the UI without the UI having to know that BLABLABLA means "can't connect to database".

Of course, exception handling can get messy too, and can make the code less readable, so you really want to use it only for handling errors, not edge cases.

EDIT

(I assume you're using PHP, so have made the links specific to that language)

missing ID field sounds like a classic argument exception - the code which called this method did so with an invalid argument. Your function cannot expect to handle this gracefully, so it's a legitimate exception.

problem updating data sounds like a database exception - you've run an SQL statement, but got back an error message. Your method cannot do what it promises to do, and has no obvious way of recovering. Probably a regular exception.

invalid status - may be another invalid argument, or perhaps some kind of logic problem. Again, your function cannot handle this, and it's not an edge case of the way the function would normally act.

Upvotes: 1

Related Questions