Writing classes - good practice [closed]

Question

Closed. This question is off-topic. It is not currently accepting answers.

---

Want to improve this question? Update the question so it's on-topic for Stack Overflow.

Closed 8 years ago.

Improve this question

First thing is that I didn't really want to post this one on stack code exchange, because this is really small amount of code written in about 5 mins.

I want to ask you if the class (my first in c++) I written is acceptable. I didn't really se a lot of c++ code so I can't compare this to anything.

But I've seen some classes implementing only function declarations, the interior of these function was written somewhere else in the code.

I'm asking you for any suggestions if there's something you thing is wrong. And why do they do as I described in paragraph above? Which one coding style is better?

class File {

private:

    FILE *_handler;
    char *_path;
    long _size;

    void setHandler(char *mode)
    {
        this->_handler = fopen(this->_path, mode);
    }

public:

    File(char *path)
    {
        this->_path = path;
    }

    size_t read()
    {
        this->setHandler("r");
        char *buffer = (char*) malloc(sizeof(char)*this->_size);

        return fread(buffer, 1, this->_size, this->_handler);
    }

    void write(char *data)
    {
        this->setHandler("w");
        fputs(data, this->_handler);
    }

    long size()
    {
        if(! sizeof(this->_size) > 0)
        {
            fseek(this->_handler, 0, SEEK_END);
            this->_size = ftell(this->_handler);
            rewind(this->_handler);
        }

        return this->_size;
    }

}; // End File

Answer 1

There are technical problems here and what I view as fundamental design problems.

Technical:

How many times do you open a file? How many times do you close one? Look at what read() and write() do.

Where is the error handling? What happens if an fopen() fails. Never use return values without checking them.

Fundamental design problems:

You allocate memory, who frees it? It's usually a bad idea to separate responsibility for allocating and freeing. C++ folks tend to use smart pointers to help with this.

What would your code do if given a really big file?

Most fundamental of all: your interface is a "you must remember this" interface. What happens if someone calls read() without remembering to call size() first? Why should your caller need to do that? Design your interface with the intent of making your caller's life simple.

Answer 2

There is no need to use this if there is no ambiguity.

File(char *path)
{
    _path = path;
}

Same thing with functions you can drop this

size_t read()
{
    setHandler("r");
    char *buffer = (char*) malloc(sizeof(char)*_size);

    return fread(buffer, 1, _size, _handler);
}

Implementing functions in class declarations has it's uses but it is not a must (unless using templates), you can define your functions in a source file and include your class header file from there. This keeps the class implementation separate from the class interface.

Imagine you change one function's implementation in the header file, all the files including this header even if they are not using that function will need a recompile.

Since you are using C++, you might want to look into using the c++ file objects (fstream,ifstream,ofstream etc.).

And finally I don't really see the point of wrapping a file like this unless your class is providing some extra functionality, all you did here was change the name of the functions and created another layer of abstraction which doesn't bring much to the table.