Firstly you should check return values for allegro routines, particularly set_gfx_mode: for me this fails with the given 800x800 size and the program crashes later at the call to blit, since screen is NULL. (Works at 800x600.)
Next it is common to separate logic and drawing. The update methods generally should only deal with the logical state of the program, and the drawing should be handled by separate methods (this should help later when you want to start timing the program, and for example, when you can't draw things in the same order as you update them). That would also be in line with the OOP guideline that each method should do one task (updating logic and drawing are two separate tasks).
#SelectExpand
1void Ant::move() {
2 switch(this->direction) {
3 case 'N':
4 this->set_position(this->pos_x,this->pos_y-1);
5 break;
6 case 'E':
7 this->set_position(this->pos_x+1,this->pos_y);
8 break;
9 case 'S':
10 this->set_position(this->pos_x,this->pos_y+1);
11 break;
12 case 'W':
13 this->set_position(this->pos_x-1,this->pos_y);
14 break;
15 }
16}
17
18void Ant::turn_left() {
19 switch(this->direction) {
20 case 'N':
21 this->set_direction('W');
22 break;
23 case 'E':
24 this->set_direction('N');
25 break;
26 case 'S':
27 this->set_direction('E');
28 break;
29 case 'W':
30 this->set_direction('S');
31 break;
32 }
33}
I believe it greatly simplifies code to represent direction as a vector (increment along x and y axis). Then move would become:
pos_x += increment_x;
pos_y += increment_y;
and left and right turns would be (from memory, and using Python syntax, in C++ you'd need a temporary variable):
increment_x, increment_y = -increment_y, increment_x #left
increment_x, increment_y = increment_y, -increment_x #right
As to OOP, it is common not to give access to data members (make them private or protected) - although in a small and simple program such as this it might not matter much.
You might also make use of constructors, e.g instead of
this->ant.set_position(400,400); // Center of view
this->ant.set_direction('W'); // Becase first field is white, so turn left
... Ant might do that in the constructor and System might have a constructor that does:
System::System():
ant(400, 400) //I suppose Ant could figure out the initial direction itself
//...
{
//...
}
The code also produces a couple of "comparison between signed and unsigned expressions" warnings. There are fixed by making these unsigned, as you'd be comparing them against unsigned loop counters:
const unsigned FIELD_SIZE_W=800;
const unsigned FIELD_SIZE_H=800;
... and personally I'd reserve ALL_CAPS only for preprocessor macros.
As to defensive programming, you might use assert for preconditions (e.g array indexes are not out of bounds) - this might have caught your earlier problems.
And using this-> everywhere tends to be just noise.
I can cite 2 projects where user safety was more important than programmer convenience
Well, generally my point was that if you have foo unintentionally modifying the loop counter:
for (int i = 0; i != N; ++i) {
foo(i);
}
... then changing != to < neither addresses the problem not makes the program any safer. It is indeed rather a suggestion to use a "on error resume next" kind of safety net, which just happens to mask the real bug (you need to get at least simple, "iterate from 0 to N" -loops right, if you want any reliability from the program).
]]>
