6

I have the following situation.

I'm writing a c++ program where I have a function that checks a custom equivalence of two json objects. If the check fails, I don't want the entire program to stop (its a QT gui program), and I have a branch that executes upon receiving an exception. This exception is then passed up to QT where a the qt gui displays an error message from e.what().

In order to do this I made a function that took care of my exception throwing for the equivalence comparisons. it looks something like this:

void makeComparison(const json& a, const json& b){
     if(condition fails)
        throw (error)
     if(other condition fails)
        throw (error)
     ...
}

I debated just making this a boolean function instead, however I wanted verbose error messages to tell exactly what went wrong in the program to the user. It seems odd to have a function that doesn't do anything but throw errors on comparison failures, and then having to catch those and keep throwing them upward. I feel like there's got to be a better way to handle this. Here is ME of what the rest of the program looks like.

// some member function
try{
    makeComparison(a, m_b);
    // do something
}catch (exception e){
   // do soemthing else
   throw(e);
}
// in gui
try{
    object.exceptionThrowingCode(a);
}catch (exception e){
   QMessageBox::critical(this, tr("Error"), tr(e.what());
}
  • Theorically, yes. Using exception for control flow is consider bad practices. It's functionally equivalent to goto. It may interest – Laiv May 17 '17 at 18:21
  • @Laiv, while I see how this is exactly using exceptions as control flow (with the rest of my code), I don't see how to avoid exceptions here, unless I manually want to pass up a string/store it in my object to see if it resulted it any errors, I don't see how this is the "better" solution either. – opa May 17 '17 at 18:33
  • Some improvements are noticeable in the long run. The answer that caused the mess with the actual one was pointing to some of the wakeness of the actual design. I can post it up again if you didn't read it. – Laiv May 17 '17 at 21:04
  • 1
    Why are you catching and re-throwing the exception? If you can just use RAII instead of doing that manually, it reduces coupling between makeComparison and its caller. – Useless May 18 '17 at 12:29
  • 1
    You missed the if ... it's cleanup code. If it isn't cleanup code, it may be less simple, or not appropriate. Since you didn't show what it is, there was no way to tell in advance. Anyway, even if it's simple to implement, it can be too large to fit in a reasonable comment, especially since I have no clue how much or little experience you have of those techniques. – Useless May 31 '17 at 8:27
3

In your example, your comparison and handling procedures occur very close to each other. Usually, exception throw/catch is intended to handle the converse situation, where things are far away. You don't really need to throw the exception from makeComparison(), you can return a Cause value. If the Cause is OK then proceed with your happy path, otherwise you can switch on the Cause to report and recover, and then throw it to the GUI.

This takes the "exception as flow control" out of the picture, until it is needed by the messaging requirements of Qt.

Edit: By the way, Cause is simply a way to encapsulate a return code in a sensible fashion. In some environments, I create a Throwable sub-class to hold status information, so that I am able to actually throw it when needed.

  • While this certainly is an alternative to the odd exception only function, I'm not certain that this is solution is "good code" either in this context. The problem is that I want to put contextual information in the messages, so Cause cannot be a simple enum, additionally I need this data to go to a separate QT window object, would I have to test if this Cause object is OK all the way up? then throw at the last level? Even though I have no return for object in my example, I could have just as easily had a normal non error related return. – opa May 17 '17 at 18:45
  • I notice that what is actually happening in my code is I know exactly how to handle the branches of the code at the level errors occur, so that that level there isn't a need for exceptions at all, the issue is that I need a way to pass what happened to cause certain branches to a separate object to be executed with out halting returns in the first object. That makes me think I should actually be storing error strings inside the object used in my example to then be retrieved later and conditionally displayed. – opa May 17 '17 at 19:06
  • You are mainly doing "error-driven-design". I would find to be hard to maintain a code which readability is based on hopping from one class to ... Which of the classes did you say will catch the exception? – Laiv May 17 '17 at 20:05
  • 1
    @Laiv I think you just edited your answer into his, it completely changed what he was saying. – opa May 17 '17 at 20:34
  • @Laiv, aren't you going to post your own answer? – opa May 17 '17 at 21:05
3

Is it bad practice to have functions whose sole purpose is to throw errors?

To me the method makeComparision() is implementing exceptions for control flow, and that is considered anti-pattern by many. But, as usual, the suitability in the software engineering is tied to needs and requirements that vary among projects

Anti-patterns aside, the implementation raises other concerns, I would like to highlight

Semantically misleading

From the point of view of a developer, a block of code which main responsibility is to look for discrepancies in the state of the data and to raise errors is usually considered a validator rather than a comparator.

Doubtful readability

Due to the misleading name and the actual signature, we are forced to look inside the method in order to understand its function. Take a look to the Principle of least astonishment, It may interest you.

Additionally, the traceability involves hops from one class to another. The execution sequence becomes blurred.

Complexity

From the point of view of the maintainability, a method composed basically by if/else blocks tends to add cyclomatic complexity. It also contributes to the doubtful readability.

It generates technical debt which is often a sign of a poor design. In a long run, the technical debt becomes a real problem that often constrains the way the system evolves.

Proposals

Consider the next suggestions

Returning a valid response

Allowing to know what happened during the comparison in a graceful manner could be a big improvement to the design. It addresses some of the concerns mentioned above too.

A possible candidate (wrote in java ):

public class ComparisionReport {

    private Collection<String> differences;

    public ComparisionReport(Collection<String> differences){
        this.differences = differences;
    }

    public boolean areDifferents(){
        return !this.differences.isEmtpy();
    }

    public boolean areEquals(){
        return this.differences.isEmtpy();
    }

    public String [] getDifferences(){
        return this.differences.toArray(new String []{});
    }
}

Separation of conocerns

A comparator doesn't necessarily need to know that different JSONs causes errors. So, why don't we delegate the result validation to another component?. For instance, the makeComparison() caller. It reduces the method's complexity, improves the readability and the maintainability.

If these changes are not possible, consider changing the name to validationByComparision(). It makes more sense accordingly with its actual implementation.

  • I could see uses in some cases, though. Eg, a validation function where the direct caller isn't gonna be catching those exceptions, anyway. The issues with readability can be resolved by having better names. Eg, throwAppropriateError or checkForAndThrowErrors. I could also see such a function used, say, to wrap exception handling (eg, you received an exception from some component and now want to throw a more specific exception based on it). – Kat May 26 '17 at 18:52
  • If we want to avoid possible confusion of the fact that the method never returns, we could also just return an exception object and let the caller throw it (possibly returning null/None/etc if no there's no errors). Although I feel a suitable name for the function (not the one OP picked) and documentation makes throwing errors like that perfectly fine. – Kat May 26 '17 at 18:53
  • My little english doesn't allow me to answer as I would like. Nevertheless, I think there's a single word that woud do it fairly good: KISS :-) – Laiv May 26 '17 at 21:16

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.