Does my code look nice?

[dhayden]

Lots of good comments here.

else
     this->curItemSlot++;

The problem with this code is that 2 years from now, some other developer will realize that you need to do more, like expanding the window, so they change it to:

else
     this->curItemSlot++;
     this->expandWindow(40);

Only that doesn't work because only the first statement is executed. For this reason, I always use braces:

else {
     this->curItemSlot++;
}

[Duthomhas]

That's a common rule of thumb, but I personally think it's stupid.
Anyone who blithely adds a second line like that needs to quit C++.
(IMNSHO.)

[edit] Realized I need one of these: http://xkcd.com/481/ (Becuase the next post makes it really clear I do.)

[danielmccarthy]

Duoas your comment is negative. People learn from mistakes this is how people progress.. Maybe it is you who should quit C++ if you think that negatively about progress. Practice makes perfect that is why I posted my code here for feedback.

Thanks for your reply dhayden I will keep that in mind while writing my code in the future

[Duthomhas]

/me feels bad
You are right. That is really negative! Sorry. (When I wrote it it didn't seem so rude... shows how brainless I was being.)

Yes, a lot of people feel that rule of thumb is a very Good Thing.

I still don't, but only because it seems to me to be a real obvious error. But, having said that, I've made all kinds of really obvious errors before...

Sorry again.

[danielmccarthy]

No problem mate :)

[dhayden]

I still don't, but only because it seems to me to be a real obvious error. But, having said that, I've made all kinds of really obvious errors before...

It's an obvious error, but I've seen excellent programmers with more than 10 years of experience make this mistake. We are all only human. A really good program is maintainable, and that means that the code is structured in a way that makes it hard to make mistakes when modifying it. In other words, you want to avoid conditions where a change "here" will break something "there."

I'll give an example (and apologize for hijacking this thread). This is from the library for the AP C.S. course many years ago when it was taught in C++. They had a vector (okay, it was really a template) declared like this:

class vector {
    unsigned size;
    int *data;
public:
    vector(int sz);
};

and in another file:

vector::vector(int sz) :
    size(sz),
    data(new int[size];
{;}

This works, but if I saw it in a code review at work, I would insist that it be changed. The reason is that the size and data members get initialized in the order that they are declared, not the order that their initializers appear in the constructor. So suppose someone decides to swap the position of size and data in the declaration:

class vector {
    int *data;
    unsigned size;
public:
    vector(int sz);
};

simple right? What could go wrong? Only now the constructor, which isn't even in the same file, breaks because instead of calling size(sz) and then data(new int[size]), it calls data(new int[size]) first, and since size is uninitialized at that point, who knows how much space will be allocated.

Even worse, this is the sort of bug that might get past regression testing if it happens to allocate enough space in the test program.

The change is to have the initializers independent of the order in which they are called:

vector::vector(int sz) :
    size(sz),
    data(new int[sz];
{;}

[JLBorges]

> I've seen excellent programmers with more than 10 years of experience make this mistake.

You should ask them to use slightly more sophisticated tools; for instance an editor that auto-indents.

So that if '2 years from now, some other developer will realize that you need to do more, like expanding the window, so they (try to) change it to:',

else
     this->curItemSlot++;
     this->expandWindow(40);

they immediately know that they have made a mistake (because they will see):

else
     this->curItemSlot++;
this->expandWindow(40);

A better coding style would also have helped:
else this->curItemSlot++;


> The change is to have the initializers independent of the order in which they are called:

A simple solution is not to use the member-initializer-list for non-const scalars.

vector::vector( /*int*/ std::size_t sz ) 
{
      size = sz ;
      data = new int[size] {} ;
      // there are no surprises here
}