HiveBrain v1.2.0
Get Started
← Back to all entries
debugcppMinor

Error-Handling Class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
handlingerrorclass

Problem

I have been doing some searching into how to properly handle errors in C++. I have found that the two most common techniques are exceptions and enumerations. I have created a simple class to expand on the enumeration method. It's main features are boolean operator overloading to return if the object actually has an error "attached to it", and the ability to set messages at the point where the error occured. It uses an enumeration so that default error messages can be set. The ones in my code are for my current program, and are meant to be examples. Here is my code:

```
enum Err_Type {
NO_ERR, EMPTY, INVALID_INPUT, OUT_OF_RANGE,
INVALID_FUNC_NAME, INVALID_PARANS, INVALID_OPERATOR, DIV_ZERO, INVALID_BASE,
FILE_IO_ERROR, CUSTOM
};

class Error
{
public:
// Constructors
Error() { Type = NO_ERR; Message = ""; }
Error(Err_Type type) { Type = type; SetMessage(type); }
Error(std::string msg) { Type = CUSTOM; Message = msg; }

// Operator Overloading
explicit operator bool() const { return Type != NO_ERR; }
bool operator !() const { return Type == NO_ERR; }
void operator ()(Err_Type type) { Type = type; SetMessage(type); }

// Public Methods
void ChangeMessage(std::string msg) { Type = CUSTOM; Message = msg; }

Solution

Orthography

I guess it is a typo, but INVALID_PARANS should probably be called INVALID_PARAMS.

The message for INVALID_PARANS also contains the word "parantheses", which ought to be "parentheses".

Style

Usually, it is preferred to have every enumerator on it's own line. This also applies to methods, meaning that you should have every statement on it's own line, e.g. your ChangeMessage method should become something like

void ChangeMessage(std::string msg) {
    Type = CUSTOM;
    Message = msg;
}


Also, you should start indenting your methods. Everytime you enter a new block (= write a new {) you should go one level of indentation deeper, i. e. add a tab or 2, 4, or 8 spaces (according to your personal preference). This means that the SetMessage method should look more like this:

void SetMessage(Err_Type type) {
    switch (type) {
        case EMPTY:
            Message = "Entry cannot be empty!";
            break;
        ...
    }
}


(Also applies to all other methods)

Another important point is your naming. Although there are no fixed rules for this in c++, you'll usually see member variables, methods and class names beginning with a lowercase letter. Also, if you have compound names, most c++ programmers prefer the so called snake_case style, meaning that each part of the name should be written in lowercase and joined with an underscore (this style is used throughout the standard library, but there are other possibilities, such as camelCase, if you are uncomfortable with it). Most important, however, is that you stay consistent. Thus Err_Type should either be called err_type or errType in line with the style you choose.

const-Correctness

Since GetErrMessage and DisplayMessage do not alter any member variables, they should be marked const.

Improvements

Since you tagged this question with c++11, I advise you to use enum class instead of enum to keep your global scope clean, since enum just puts all enumerators into global scope. Also, your implementation currently lacks a method to get the the actual Err_Type that an Error object represents.

GetErrMessage returns a string by value, which means making a copy of it everytime the method is called. Most of the time, returning a const std::string& would be the better choice here.

Since this class handles errors, printing to std::cerr instead of std::cout would be the appropriate behaviour for DisplayMessage. However, this method shouldn't actually exist in the first place, since it violates the SOLID principles. What if somebody wants to output an error into a file? Bad Luck. Also, it is highly discouraged to use std::endl in most cases, since it not only writes a line terminator but also flushes the buffer, which can lead to severe performance loss (assuming that the endl in your code is indeed std::endl).

Furthermore, the method ChangeMessage is a bit misleading: It does not only change the message, but it also silently changes the error type to CUSTOM. You should either change your method to allow custom error messages for all kinds of errors or rename it to reflect what it does.

Code Snippets

void ChangeMessage(std::string msg) {
    Type = CUSTOM;
    Message = msg;
}
void SetMessage(Err_Type type) {
    switch (type) {
        case EMPTY:
            Message = "Entry cannot be empty!";
            break;
        ...
    }
}

Context

StackExchange Code Review Q#157448, answer score: 5

Revisions (0)

No revisions yet.