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

Critique my first C++ "Hello World" program

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

Problem

This is my first attempt to write a simple C++ program:

#include 
#include 
#include 
using namespace std;

class Book {
    public:
    char title[30];

    Book(char *tit)  {
        strcpy(title,tit);
    };

    char *gettitle() {
        return title;
    }
};

int main() {

    Book miolibro((char *)"aaa");
    cout  vettorelibri; 
   for (int i=0; i<10; i++) {
        Book l( (char *)"aaa" );   // i tried something like "aaa"+i but it does not work

        vettorelibri.push_back(l);   
    }

    for(int i=0; i<10; i++) {
        cout << vettorelibri[i].gettitle() << endl;
    }
};

Solution

It has been commented that when working in C++ you should use std::string. Assuming you want to stay with C style strings (or at least understand how you ought to work with them, in case you ever have to), I have the following suggestions:

strcpy(title,tit);


tit can be larger than the buffer title allows.[1] In that case, your program will crash or worse, continue running while corrupting some other part of memory. To avoid that you should always use functions which let you track the size of the destination buffer and detect overflow. For example:

strncpy(title, sizeof(title)-1, tit);
title[sizeof(title) - 1] = 0;


This has some undesirable traits in that it will silently truncate the string if the input is larger than the destination. If you are working on a system that has the non-standard strlcpy, that has some cleaner error conditions:

if (strlcpy(title, sizeof(title), tit) >= sizeof(title)))
{
    // TODO: decide how to handle the error (in your case maybe throw
    // an exception)
}


Moving along the spectrum (from code that crashes given large input, to code that truncates the input, to code that errors out when the buffer in insufficient...), you might also want to dynamically allocate memory for this.

char *title;

Book(const char *tit)
{
   title = strdup(tit);
   if (!title) { throw std::bad_alloc(); }
}


This creates complications if a Book object is ever copied, so you need to handle that:

Book(const Book &b)
{
   title = strdup(b.title);
   if (!title) { throw std::bad_alloc(); }
}


And it needs to be freed when your object is destroyed:

~Book()
{
   free(title);
}


If this seems too complicated it's because for higher-level C++ code it probably is. std::string will take care of all of this for you. Note though that the memory layout of your object will be substantially different with these dynamic-memory approaches.[2]

Next up...

Book miolibro((char *)"aaa");


As a general rule, having to do casts like this is often an indicator that you're doing something wrong. :-) When writing code and reviewing the work of others' I tend to try to avoid as many unnecessary casts as possible. A lot of times these mask bugs, for example casting a function to an incorrect function pointer type can produce crashes, or casting away const may crash when someone tries to modify a read-only buffer. For reasons like this it's a good habit to watch for inappropriate casts when reading code, and be sure not to introduce them yourself; code like this should offend your sensibilities. :-) [3]

In your case, the issue is that string literals have the type const char*, so the compiler complains that you are passing this to something that doesn't have const. So what you need to do here is:

Book(const char *tit)
{
    // ...
}


Footnotes and tangential comments:

[1] If there are any English speaking people who might read your code, I suggest you come up with a better name than tit for that variable.

[2] In the earlier example, characters of title exists alongside any other members of the class. When using dynamic memory, there's now an extra pointer dereference going on to get to the characters. In most cases this won't make a difference but you can think of theoretical cases where the former is desired over the latter.

[3] Side note: In my experience one way to tell that someone doesn't know what they're doing in C/C++ is the use of casts. A sloppy programmer will write code that the compiler rejects, and then to get it "working" quickly they'll introduce casts to silence compiler warnings instead of fixing the actual problem. If you are working on a team where people do this, I find it's sometimes a good indication to be skeptical of that person's work for other reasons.

Code Snippets

strcpy(title,tit);
strncpy(title, sizeof(title)-1, tit);
title[sizeof(title) - 1] = 0;
if (strlcpy(title, sizeof(title), tit) >= sizeof(title)))
{
    // TODO: decide how to handle the error (in your case maybe throw
    // an exception)
}
char *title;

Book(const char *tit)
{
   title = strdup(tit);
   if (!title) { throw std::bad_alloc(); }
}
Book(const Book &b)
{
   title = strdup(b.title);
   if (!title) { throw std::bad_alloc(); }
}

Context

StackExchange Code Review Q#3123, answer score: 13

Revisions (0)

No revisions yet.