What is the better way of getting different outputs for two very simmilar related workflows

Question

I have a very concrete scenario but I think it would be quite frequent.

I'm trying to come up with a solution that it's "better" by better meaning more scalable, reusable and not smelly.

I'm working with very old crufty legacy code and Trying to improve it one thing at a time. Right now I have a process in which users can "vote" "things".

The process to determine if a user can vote is a complex logic and it may be subject to change.

Right now I have two functions, one (call it can_vote() ) takes the user and the thing and returns true or false. It's used to know what to display to the user.

The other one is vote() It takes user and thing and and checks again all the logic that can_vote checks but instead of returning false on error throws an error that gets displayed to the user.

The system tries to never get to a state in which the user would be able to use "vote" if the result would be an exception but it still happens.

I know there may be a thousand flaws to the whole system but right now the only thing I'm trying to do is unite the logic of the two functions in one single place in the better way (as defined above).

Ideally I would like to be able to call can_vote in my controllers just getting a true or false and acting acordingly. can_vote, being a binary question should return true|false. I could be able to use the logic inside can_vote in places where I want to display information about "false" responses.

I've got multiple ideas about how to solve that but all of them seem... weird:

Possible solutions

a) use can_vote(user,thing,verbose)

verbose beeing true false if true returns string with the error if false returns true|false

public can_vote ( user, thing, verbose ) {
    if( user is not logged ) {
        if( verbose ) {
            throw user is not logged
        }
        return false
    }

    //all other checks
    return true
}

cons

• returns different types depending on parameters
• you get an if else that you carry arround the function

b) use a private function to determine if can vote. Use two helper functions

that would be something like that:

private can_really_vote ( user, thing ) {
    if( user is not logged ) {
        throw user is not logged
    }

    //all other checks
    return true

}

public can_vote ( user, thing ) {
    try {
        can_vote = this->can_really_vote( user, thing )
    }
    catch {
        //can_really_vote threw an exception
        return false
    }

    return false
}

public vote ( user, thing ) {
    if ( this->can_vote( user, thing ) ) {
        //do the vote thingy
    }
    else {
        // you shouldn't even get here. The user should have gotten an exception from can_really_vote
    }
}

c) what php usually does

That would be something like

public vote () {
    if ( can_vote() ) {
        //carry on with the voting
    }
    else {

        display can_vote_last_error()
    }
}

Things like that are usually seen in database access classes and the like. Really? Keeping an internal trace of all those silly errors like "you are not logged", "the contest is over" and all that stuff just to be able to recall them? It seems quite complicated.

Of course It can be done better, for example with dedicated exceptions... doesn't seem right dough.

I'm sure I can get more solutions... the problem boils down to wanting to get a true/false result in one case and some text on another for the same logic.

I guess the problem is quite common so there should be some software pattern for this. Or maybe I'm being unable to abstract from it so I see the obvious elegant solution.

What would you do?

Thanks!

PS I'm almost sure it doesn't matter but I'm working in vanilla PHP 5.3

Answer 1

I would recommend your solution c), but I would change it a bit: I would not display some error from the can_vote function. The user should know long before that if he can vote or not and why. The vote() function only checks if the user can vote as a safeguard, because as you said yourself there should be NO WAY your program ever calls vote() when the user is not allowed to vote. So just throw an exception or log an error.

A few comments on your other solutions:

I consider a) do be very confusing and very risky, because if you accidentally flip the verbose parameter and check the result for true/false you might think everything went fine even though you got an error message. Try debugging that problem - a nightmare!

Solution b) is just bloating of code, because all you do is wrap one function call in a try/catch and make it another function. Also, imagine a new developer in your project wants to know if a user can vote. He finds the two functions can_vote and can_really_vote - which one is he supposed to call? Needlessly confusing and no advantage whatsoever.

Answer 2

I would recommend (d) have your voting function return a result saying whether it was successful or not. Whatever mechanism you use for voting can fail, the user should be informed if that happens.

public bool vote() {
   if (!can_vote()) { return false;}

   // take whatever action you need to, prefer functions that signal failure
   return true;
}