7

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());
}
Krupip
  • 1,340

2 Answers2

5

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.

BobDalgleish
  • 4,749
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 ComparisonReport {

private Collection<String> differences;

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

public boolean areDifferent(){
    return !this.differences.isEmpty();
}

public boolean areEqual(){
    return this.differences.isEmpty();
}

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



}

Separation of concerns

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.

Krupip
  • 1,340
Laiv
  • 14,990