Abstraction for an IO device

I have a C++ class which acts as an abstraction for an IO device which is controlled through file descriptors. As I am very new to C++, I would be glad if you can give me devastating feedback on things I've done wrong.

This especially covers:

• Code style

• Design patterns

• Error handling

• Things which are open for failures

• C++ approaches which can be used over C approaches (file IO)

gpio.h

#ifndef SRC_GPIO_H_
#define SRC_GPIO_H_

#define GPIO_IN     0
#define GPIO_OUT    1

#define GPIO_LOW    0
#define GPIO_HIGH   1

class GPIO {
    public:
        explicit GPIO(int id);
        ~GPIO();

        int  Value();
        void Value(int value);
        int  Direction();
        void Direction(int value);

    private:
        int _id;
        int _value_fd;
        int _direction_fd;

        bool Exists();
        void Export();
        void Unexport();
        void OpenValueFd();
        void OpenDirectionFd();
        void CloseValueFd();
        void CloseDirectionFd();
        void SeekToTopOfValueFd();
        void SeekToTopOfDirectionFd();
};

#endif

gpio.cc

```
#include "../src/gpio.h"

#define GPIO_PATH_EXPORT "/sys/class/gpio/export"
#define GPIO_PATH_UNEXPORT "/sys/class/gpio/unexport"
#define GPIO_PATH_DIRECTORY "/sys/class/gpio/gpio%d"
#define GPIO_PATH_DIRECTION "/sys/class/gpio/gpio%d/direction"
#define GPIO_PATH_VALUE "/sys/class/gpio/gpio%d/value"

GPIO::GPIO(int id) {
Export();

OpenValueFd();
OpenDirectionFd();
}

GPIO::~GPIO() {
CloseValueFd();
CloseDirectionFd();

Unexport();
}

bool
GPIO::Exists() {
char * path;

if (asprintf(&path, GPIO_PATH_DIRECTORY, id) result++;
}

void
GPIO::Export() {
char * id;

if (Exists()) return;

if (asprintf(&id, "%d", _id) < 0)
throw "Error converting id to char.";

int fd = open(GPIO_PATH_EXPORT, O_WRONLY);

if (fd < 0)
throw "Error opening GPIO export.";

if (write(fd, id, strlen(id)) < 0)

Prefer the C++ way

As you seem to expect, one of the key issues with your code is that it's not very C++y.

1. Prefer std::string over C-style strings.

std::string is better in almost every way. It takes care of its own memory and grows when needed. The speed is usually comparable to C-style strings (sometimes faster, sometimes slower). The code written using string is often more readable. For example, compare your code:

if (strncmp(data, "in", 2) == 0)

with the std::string variant:

if (data == "in")

1. Prefer iostream to FILE*.

The C++ library for IO is typesafe, extensible, object-oriented and should be preferred. Use an ifstream to read from a file and ofstream to write, or fstream for both.

1. Consider throwing exceptions derived from std::runtime_error and std::logic_error

By having an exception hierarchy, you can have fine grained control over which errors to catch where. Your code will also play nice with others, who might expect your exceptions to behave like the standard ones. Want to catch everything? Catch std::exception. Want to catch only your own? Do so. Want to get the error message? Call .what().

Remember that exceptions should always be thrown by value, and catched (caught) by reference(-to-const).

1. Prefer enum or consts to #defined constants.

Instead of

#define GPIO_IN     0
#define GPIO_OUT    1

write

enum IODirection { GPIO_IN = 0, GPIO_OUT = 1 };

For your path name #defines, use const std::string instead. (And consider moving them into class scope as static, or into a namespace.)

You get type safety, easier debugging, normal scope rules; the works.

1. Avoid leading underscores in identifiers.

The following identifiers are reserved:

• Any name that begins with an underscore and is in the global namespace.

• Any name that begins with an underscore followed by a capital letter.

• Any name that contains two consecutive underscores.

While your _id and similar variables are technically correct, don't rely on yourself or others memorizing these rules. Play it safe and move the underscore to the other side: id_, or drop it altogether.

Other things

These are mostly nitpicking, but you asked for a "very critical review" :-)

1. #include the proper headers.

I'm guessing you already are, and that maybe they were removed when posting the code for review. I'm mentioning it just in case. It's nice to include #includes in the review code, too.

1. Let the build system take care of include paths.

Instead of: #include "../src/gpio.h", prefer to just use #include "gpio.h" and let the preprocessor and build system take care of finding the right header for you. This makes it a lot easier to move files at a later point.

1. Don't cast unless you have to.

Instead of

return static_cast result++;

write

return result++ != 0;

(And use parenthesis on your casts.)

1. Use fclose() instead of close().

The C library function is called fclose(). (This is a moot point, however, since you should prefer C++ IO anyway.)

1. Use descriptive names and a clear interface

Your public interface functions have names that imply that they don't change the state of the class. If they don't, make the functions const. (Your Exists() function should at least be const. The same may apply to functions that are only reading.) Consider separating the state changing functionality from the non-state-changing functionality, and putting the latter into const functions.

Rename functions that do change state to reflect that.

Finally, data is a classic, horrible variable name. I'm sure you can do better!

StackExchange Code Review Q#28455, answer score: 14