patterncppModerate
Critique my first C++ "Hello World" program
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
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
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.
This creates complications if a
And it needs to be freed when your object is destroyed:
If this seems too complicated it's because for higher-level C++ code it probably is.
Next up...
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
In your case, the issue is that string literals have the type
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
[2] In the earlier example, characters of
[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.
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.