5
\$\begingroup\$

My goal is to make a consistent and easy way to determine how a function fails/succeeds. I'm looking for code review of my Status class for any bad practices, code that isn't thread-safe and general improvements and changes.

For logging and debugging I always needed to know how a function failed and print this out as readable text. The solution I usually employed was using enumerations - enum ERROR_CODE {..}; - which is fine except they cannot be expanded/overridden - I'm making an API so enums don't allow the user to expand/add custom error codes. They also don't print out nicely to describe what error actually occurred - they just print out as numbers. So I made a status class.

Main features:

Allows creation of error/states wherever & whenever (runtime, compile time):

static const Status S_SUCCESS         = CStatus::registerState("Success");
static const Status S_EMPTY_CONTAINER = CStatus::registerState("Empty rectangle vectors");

General usage:

Status unionRectangles(const vector<Rect>& left, vector<Rect>& right)
{
  if (left.size() <= 0 || right.size() <= 0)
     return S_EMPTY_CONTAINER;

  ...

  return S_SUCCESS;
}

Print out descriptive errors without requiring conversion:

vector<Rect> v1, v2;
Status myStatus = unionRects(v1, v2)
cout << myStatus << end; // "Empty rectangle vectors"

Easily and efficiently compare objects without conversion:

if (myStatus == S_SUCCESS) // for eg; resolves to if (1 == 9), ie, not string comparison
    // do something

Easy to inline check error:

if (!unionRects(v1, v2))
    // do something

Full implementation:

#ifndef STATUS_H
#define STATUS_H

#include <iostream>
#include <hash_map>

typedef int STATE;
typedef std::hash_map<STATE, tstring>::iterator StateIterator;
typedef class CStatus;
typedef const CStatus Status;

#ifndef S_SUCCESS
#define S_SUCCESS CStatus::CS_SUCCESS
#endif // S_SUCCESS

#ifndef S_UNDEFINED_ERROR
#define S_UNDEFINED_ERROR CStatus::CS_UNDEFINED_ERROR
#endif // S_SUCCESS

class CStatus
{
public:
    // Static Properties//
    static Status CS_SUCCESS;
    static Status CS_UNDEFINED_ERROR;

    // Static Functions //
    static Status registerState(const tstring &stateMsg)
    {
        // Post: 

        int nextStateTmp = nextState + 1;
        std::pair<StateIterator, BOOL> res = states.emplace(std::make_pair(nextStateTmp, stateMsg));

        // if state has been inserted
        if (res.second)
            return Status(++nextState);

        // return existing state id
        return Status(res.first->first);
    }

    // Properties //
    const STATE state;

    // Functions //
    CStatus(const STATE &state)  : state(state) {}
    virtual ~CStatus() {}

    const tstring& toString() const 
    {
        return (states.find(state) == states.end()) ? states[state] : _T("Undefined");
    }

    operator tstring() const
    {
        return toString();
    }

    operator int() const
    {
        return state;
    }

    bool operator==(const CStatus& obj) const
    {
        return state == obj.state;
    }

    bool operator!=(const CStatus& obj) const
    {
        return state != obj.state;
    }

    bool operator!() const
    {
        return *this != S_SUCCESS;
    }

    std::ostream& operator <<(std::ostream &stream, const CStatus& obj)
    {
        return stream << obj.toString().c_str();
    }

    friend std::ostream& operator<<(std::ostream& stream, const CStatus& obj);

private:
    static std::hash_map<STATE, tstring> states;
    static STATE nextState;

protected:

};


//static Status S_SUCCESS         = CStatus::registerState(_T("Success"));
//static Status S_UNDEFINED_ERROR = CStatus::registerState(_T("Undefined Error"));


#endif // STATUS_H
\$\endgroup\$
-1
\$\begingroup\$

Is this a working program? In other words, have you tried to compile this? I'd be very surprised it compiled.

#include <hash_map>

This header does not exist in the C++ Standard Library. If you want a hash map, you need to use #include <unordered_map> and use the C++11 standard.

typedef const CStatus Status;

I'm pretty sure that something like: const Status S_SUCCESS doesn't compile as that resolves to: const const CStatus S_SUCCESS.

#ifndef S_SUCCESS
#define S_SUCCESS CStatus::CS_SUCCESS
#endif // S_SUCCESS

#ifndef S_UNDEFINED_ERROR
#define S_UNDEFINED_ERROR CStatus::CS_UNDEFINED_ERROR
#endif // S_SUCCESS

You have an error in the comment for the S_UNDEFINED_ERROR macro.

Your RegisterState function could also be slightly simplified:

// Static Functions 
static Status registerState(const tstring &stateMsg)
{
    // Post: 
    auto res = states.emplace({nextState + 1, stateMsg});

    // if state has been inserted
    return res.second ? Status(++nextState) : Status(res.first->first);
}

const tstring& toString() const 
{
    return (states.find(state) == states.end()) ? states[state] : _T("Undefined");
}

I'm not familiar with what a tstring is but this function should be marked static (as only static members can access static member variables) and you shouldn't be returning the string by const value. Let the caller decide whether or not they want to make it const.

operator tstring() const
{
    return toString();
}

operator int() const
{
    return state;
}

What is the point of these and what do they return? These should be removed as they aren't going to be useful to your API. The entire point of your API is to allow the user to easily create new States without having them know too many of the internal implementation details.

bool operator!=(const CStatus& obj) const
{
    return state != obj.state;
}

Implement this in terms of operator==():

bool operator!=(const CStatus &ob) const
{
    return !(state == obj.state);
}

Finally, with this setup, your user will not be able to add any states during compile time. They will only be able to add checks during runtime (most likely during program startup). You should change your description to reflect this.

\$\endgroup\$
  • \$\begingroup\$ "as only static members can access static member variables" — You've got that backwards. Only non-static member functions can access non-static data members; static data members (and functions) are accessible with or without the this pointer. \$\endgroup\$ – Quuxplusone Dec 26 '15 at 6:37
  • \$\begingroup\$ Multiple const qualifiers (or volatile) are allowed via typedefs. You can't write const const int, but typedef const int cint; const cint foo; is ok. \$\endgroup\$ – Mat Dec 26 '15 at 8:37

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.