Writing classes - good practice

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

Try it on codeReview: http://codereview.stackexchange.com/?as=1 — Sam, Apr 11 '13 at 20:17
*"And why do they do as I described in paragraph above?"* What you're referring to is separation of interface and implementation. See here for why this is a good idea: http://stackoverflow.com/questions/333889/in-c-why-have-header-files-and-cpp-files — JBentley, Apr 11 '13 at 20:18
One comment I have, not so much about class design, is that you're mixing C code with C++ code. Things like `char*`, `fread`, `fputs`, etc. should be replaced with their C++ equivalents. — JBentley, Apr 11 '13 at 20:21
One more thing don't use _name. Leading underscores are reserved for library implementations. — , Apr 11 '13 at 20:37
Tip: when designing a new class, also design a little bitty program that uses your class. Doing so would have _immediately_ revealed several major design flaws. — Mooing Duck, Apr 11 '13 at 20:44

score 7 · Accepted Answer · answered Apr 11 '13 at 20:28

7

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.

answered Apr 11 '13 at 20:28

djna

54,992
14
74
117

I can't believe you noticed all of that and failed to notice that (A) `size` doesn't work unless `read` or `write` has been called first, and (B) `read` doesn't give the caller the data that was read, only the return value (C) The file is never closed. – Mooing Duck Apr 11 '13 at 20:42
I have to open file that many times because each time I open it with the different mode. – user2252786 Apr 11 '13 at 20:48
You may have to open the file many times, but you must close it again or you will run out of file handles. The point of my question is that you have a different number of opens and closes. (Mooing, my first technical point covers lack of close - good catch on the others.) – djna Apr 11 '13 at 21:26

score 3 · Answer 2 · answered Apr 11 '13 at 20:21

3

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.

answered Apr 11 '13 at 20:21

Barış Uşaklı

13,440
7
40
66

1

You don't need to use `this`, but it makes your code more clearly because when someone is reading the code they can immediately tell that this object is a member of the class. I have founded it to be helpful in the past when I'm reading someone else's code. – Caesar Apr 11 '13 at 20:30
1

I don't see how `this` makes anything clear in this case, it just adds noise. Adding `this` in front of every member isn't going to make the functions clearer, the way to make functions and code clear is by having well defined names and short functions that only do one thing. And if you prefix your members with `_` you almost never need `this` – Barış Uşaklı Apr 11 '13 at 20:34
2

@caesar If the code is well designed (short, well scoped functions, self-documenting symbol names, sparse use of globals), then it should be obvious from context whether a variable is a member or not, making `this` unnecessary. – JBentley Apr 11 '13 at 20:35
Maybe not in this case, but I believe it is a good habit to get into for the reasons I said in my previous post. – Caesar Apr 11 '13 at 20:35
@JBentley You must be one of the few who works in a team where everyone writes poems with their code. For me, I work with a lot of people with different styles (and nothing wrong about that) and we can't agree on everything but something like the `this` keyword is an easy one to agree on. – Caesar Apr 11 '13 at 20:37

Writing classes - good practice

2 Answers2