Critique my first C++ "Hello World" program

Question

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

#include <iostream>
#include <cstring>
#include <vector>
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 << miolibro.gettitle() << endl;

   std::vector<Book> 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;
    }
};

Answer 1

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.

Answer 2

I know I'm a little late, but let's take a look.

using namespace std;

There's no need for this. Really. You're already qualifying most of your names -- keep doing that.

class Book {
  public:
    char title[30];

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

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

As others have said, use std::string. As others have not said: definitely use std::string. You cannot sanely mix raw C strings and exceptions without smart pointers. In fact, mixing any dynamic allocation with exceptions without smart pointers is a great way to drive yourself insane, but in this case it's not even remotely justified as there's a class that does exactly what you want.

Now, let's see how Book can be rewritten:

class Book {
    std::string title;

  public:
    Book(std::string const& t) : title(t)
    {}

    std::string get_title() const {
        return title;
    }
};

Notice the funny syntax in the constructor. That's called a constructor-initializer, and it's a good idea to initialise your objects that way when possible -- it might be faster, and it makes it more explicit.

Notice that the string is passed by const reference. This means you don't copy it, but that you also can't change it -- you don't want to change it, so that's okay. It has another benefit, but I'll show that later.

Finally, we make get_title const so that you can call it on a const instance of Book. Not very important in your minimal code, but it pays off to be as const-correct as possible.

int main() {
    Book my_book("aaa");
    std::cout << my_book.get_title() << '\n';

Yay, you know about how to initialise objects that don't have a default constructor. That's a handy thing to know, and less common than I'd like to see it. Notice that we don't need the cast any more -- in fact, we don't even need to explicitly call std::strings constructor. That's because std::string can be constructed from a char const*, which "aaa" converts to. (If you're curious, "aaa" isn't itself char const* -- it's char const[4].)

You used std::endl here, but there's no need for that. You can just output a newline character, and it'll get printed eventually -- std::endl makes sure it gets printed immediately, but unless the program crashes, it'll be outputted equally soon without it as far as the user can tell.

  std::vector<Book> vector_of_books; 
  for (int i=0; i<10; i++)
      vector_of_books.push_back(Book("aaa"));

Here's code that does exactly what you were doing, but with Book's new constructor and with no temporary. I think this is just as clear, but some may disagree. An even clearer way to do this is

  std::vector<Book> vector_of_books(10, Book("aaa"));

That'll do the same thing, and describes the intent more clearly.

As an aside: you say that "aaa"+i didn't work. That is indeed something that won't work, partly because C++ doesn't provide an all-that-easy way of joining a string together with something else. Your best bet would be "aaa" + boost::lexical_cast<std::string>(i), unless you wanted to go through the trouble of explicitly creating a stringstream. There are also functions like atoi and snprintf, but those are generally less safe or harder to use.

    for (int i = 0; i < 10; ++i) {
        std::cout << vector_of_books[i].get_title() << '\n';
    }
};

This code is perfectly fine to use -- I applied some cosmetic changes, but they don't particularly matter. Gabriel is suggesting you use iterators, but I don't think those make things any better:

    for (std::vector<Book>::iterator it = vector_of_books.begin();
         it < vector_of_books.end();
         ++it) {
        std::cout << *it << '\n';
    }

The code is much longer and the bonus in efficiency isn't going to matter. Granted, with C++11 you could change the std::vector<Book>::iterator into auto, and then it would be decent, but in C++11 you could also just do:

    for (std::string const& book : vector_of_books)
        std::cout << book << '\n';

If this looks confusing, don't worry about it; it's most important that you get the basics down (like using std::string), and iterating is just a nice option that you can keep in mind for when it's more efficient/clearer.

Answer 3

If you're using C++ instead of C, you should use std::string instead of char arrays. Include <string>.

Answer 4

There are several issues:

• If you use getter method for the title in Book, you probably want to hide the title field, so make it private

• If the title length constraint for 30 is arbitrary, you might want to change the title field to a pointer to string (learn more about what kind of strings you really want here: http://en.cppreference.com/w/cpp/string)

• If you want to use the Book constructor with string literals (e.g. "aaa"), you should add const to the parameter type

• In the Book constructor strcpy is dangerous, because it doesn't check lengths of the strings (buffer overflow possible), you should better use strncpy, or if you decide to use constants, you don't have to copy at all

• It is preferable to use camelcase or underscores for separating words in identifiers for readability, so rename the gettitle to getTitle or get_title, same holds for miolibro and vettorilibri.

• If you want to avoid casting string literals, use constants instead

• If you want to pass an object of a type you defined to an output stream (e.g. cout in your case), it is preferable to overload the operator<<.

• If you use a namespace (using keyword), you don't have to type the whole names unless a name clash is possible, in your case you can just write vector instead of std::vector.

• The proper way of constructing a new object is to call a constructor using the new operator, so you should replace the Book l(...), which is implicit conversion AFAIK, with new Book(...). Than should vettorelibri.push_back(new Book("aaa" + i)); work as expected, given you switch to constants (const char* instead of char*).

• The preferred way of iterating over a vector is using an iterator instead of explicit indexing. You can read about that here: https://stackoverflow.com/questions/409348/iteration-over-vector-in-c

There might be some more problems, but this should be enough for a start:).