before I convert my Malefiz project from C to C++, I thought I should brush up my C++/OOP skills with a smaller project first, so I've converted a 1P tic-tac-toe game I wrote in C yesterday to C++.

Could someone check through it to see if there's anything that stands out as being bad coding? It works, which is good, and I get no warnings, but I'd like to make sure I eliminate any bad C habits I have before I embark on something bigger.

I've attached the code, plus a Windows binary, but I'll paste the code here as well:

main.cpp:

game.h:

game.cpp:

player.h:

player.cpp:

types.h:

defines.h:

#ifndef DEFINES_H
#define DEFINES_H


using namespace std;


#define WIDTH 172   // the width of the window
#define HEIGHT 232  // the height of the window
#define EDGE 11     // the size of the gap between the window edge and teh board
#define SQUARE 50   // the size of each square on the board
#define WHITE makecol(255, 255, 255)
#define BLACK makecol(0, 0, 0)


#endif

edit: fixed some of the code formatting.

I have some suggestions, it is just mho.

I never use "using namespace std", always use the prefix std::, because if some day you want to create some namespaces you wont get lost if there are things with the same name as std::, I took this from a book

Another thing is that I use class names starting with uppercase letters. So player becomes Player, and I could use Player player

In class I put the "private:" and "public:", it makes your class header looks better

Things you can initialize and dont need to worry if it fails or not, I usually put on the class constructor, like initialing variables. Try to avoid doing tests and using exit at constructors or destructors.

I could be wrong on some advices, I am still learning C++

I haven't really looked into namespaces yet. They didn't exist when I learnt C++.

No offense, but it's actually a pet hate of mine when people do that. By default, all members of a class are private, so adding the private keyword is superfluous.

I actually changed the game::init function to game::game just after posting that.

I do it because I prefer to put all public stuff first.

class FooBarFrob {
   public:
      FooBarFrob();
      void doSomething();

   private:
      int someVar;

};

There is some truth to that but the chances that you will write a class that conflict with the std:: namespace is quite low. Its usually more convenient to do 'using namespace std', but its your call.

I wouldn't use #define anymore. Use const int instead. The reason is just so you can have better type checking (and macros are evil).

Or enums.

Yeah, that looks OK. As Brunooo suggested, you may wish to tag C++ library functions with std:: instead of defining "using namespace std" at the start in case you want to use other namespaces.

When I create objects, I add T_ to the name of the object class and capitalize any first letters. So if I wanted to make a gamestate object class, I would call it T_GameState. 'T' stands for "Type" and is sort of a hold-over from my days programming in BASIC, not to mention a friend of mine who helped me to learn Pascal and C++ also did this.

It's almost always good to create constructors for your classes to initialize all the variables they hold. But, as for destructors, unless you do something which allocates memory from within a class object, they're unnecessary.

If you put any Allegro functions within the destructor though, make sure you don't make any global declarations of that class object, nor create them automatically within the scope of main(). If you do this, and quit the program, the destructors will be called and Allegro functions may end up being called AFTER Allegro has been removed from memory!

Other things you will need to adapt to are the C++ try/throw/catch method of error handling and the memory allocation keywords. You can use C error handling in C++ up until you need to error check C++ functions, at which point you'll need to use a "try" block to trap errors, a "catch" block to handle them, and "throw" keywords to continue passing error handling to additional catch blocks. You'll also be seeing a lot of the "new", "delete", and "delete []" keywords for allocating memory much more easily than malloc() ever was.

That's my advice. Take what you will, leave what you won't, and regardless of anything, just code the way that works for you.

So why don't you use struct instead of class?

That seemed kinda 'global variablish,' though I guess a define that everyone accesses is similar.

How so?

Like this: typedef enum { WIDTH = 200, HEIGHT = 400} window_size;?

edit: Also, I get sick of typing makecol(255, 255, 255) every time I want white, so I use #define WHITE makecol(255, 255, 255) how ould you avoid that?

Because as a long time C coder, that would just confuse me IMO a struct is for simple aggregate types, and a class is for complex types.

for that enum I'd skip the name, and make it an anonymous enum, but I'm not sure how many compilers support that (mind you I don't care, I only use GCC).

Yeah, you really can't do that with const ints afaik, as I think they need to be initialized when they are declared.. But if not, just init them after set_gfx_mode (setting the mode will generally change the format that makecol generates).

Putting aside the pointless style issues, you have a few things that concern me.

First off, though, your class design is pretty good. Your types adhere to a standard and your names are coherent and all that good stuff. So good job there.

But I think using player_type for everything is a little misleading. I would expect player_type to be a member of the player class, not something you have a 3x3 grid of in your game class.

Also, I think you could stand to break up your responsibilities a little more. The game class should be concerned with the high-level flow, and delegate to more specific classes. For example, buffer should be stored in a class like renderer or view.

I also noticed you're using an int in a lot of places where they only hold a 1 or a 0. We have a data type for that now: it's called bool.

You also have a worrying amount of inline arithmetic using numeric literals. You should replace those with constants, because a) then you know what 5 actually symbolizes, and b) if you have to change it, it's just in one spot.

You also have a worrying amount of (really long!) ifs. Some even have comments describing what it does, which is a clear sign that it needs to be refactored, probably into a function with a name similar to the comment itself.

There's also a fair amount of code that looks like it was copy-and-pasted. This is a usual sign that you need to make that into its own function. Whatever bits change from copy to copy become the parameters.

Also, when you're doing a critical action like placing a piece, you should definitely break that into its own function, because it communicates that this action is one the fundamental actions the class takes. (Make sure the function is parametrized, too!)

Allegro 5 does this in some cases. The following program won't compile in MSVC9:

#include <allegro5/allegro5.h>

#include <iostream>
using namespace std;

int main()
{
    bool test;
    return 0;
}
END_OF_MAIN()

Allegro 5 redefines "bool" to "_Bool", which conflicts with std::_Bool, which is apparently just part of MSVC's standard library implementation, since the above code compiles fine on G++.

The total namespace dump can indeed cause problems, and so should be used with caution if at all.

At least under Windows using MSVC6, the bool object type still takes up 32-bits, just like an int. There's almost no reason to use bools unless you want to force the use of true and false keywords. If the Visual C++ 6.0 optimizer is able to group bools together I've never seen it mentioned anywhere in the MSDN libraries.

Not to mention, many times when I make a variable that initially only has two states, I find later I want that variable to have more states. For example, in my current project, I have a "paused" variable. For weeks now, it only changed between 1 and 0. However, now that I've got task switching working, which automatically pauses, I had to make a way of detecting stuck keys as a result of a keyboard-shortcut task switch. I found the best way to handle this was to allow "paused" to equal 2, and to run a scan when it does and set itself back to 1 if the scan detects no stuck keys.

If I was using bools, I'd have to either change it to an int and all the true/false references to 1's and 0's, or make a new variable, take up another 4 bytes of memory, and add more initialization code for the variable and checking to make sure it's set properly.

My opinion: Stick with ints.

Fair enough. While I prefer C myself, I actually learnt C++ first, so I still see class/struct as being the same thing but just with different default permissions.

Yeah, that was a throwback to an earlier design choice. I've removed the player class.

Oh yeah! I meant to look for those and change them. Thanks for the reminder.

Fixed. That was me being lazy.

This I do intentionally as I find long ifs less confusing than nested if statements. I know most people find the opposite to be true, but it makes it easier for me. Or did you mean the longer nested ifs? I moved one of them to its own function.

I must have fixed that already, as I can't find any reperated code in the current state.

Good point. Done!

updated code:

main.cpp:

game.h:

game.cpp:

types.h:

#ifndef TYPES_H
#define TYPES_H


typedef enum { NONE, NOUGHT, CROSS } player_type;  // enumerated constants for the player type
typedef enum { GRIDSIZE = 3, NUMSQUARES = 9, TEXTLINE1 = 175, TEXTLINE2 = 190, WIDTH = 172, HEIGHT = 232, EDGE = 11, SQUARE = 50 } sizes;


#endif

defines.h:

#ifndef DEFINES_H
#define DEFINES_H


#define WHITE makecol(255, 255, 255)
#define BLACK makecol(0, 0, 0)


#endif>

graphic.h:

graphic.cpp:

mouse.h:

#ifndef MOUSE_H
#define MOUSE_H


struct position {

    int x;
    int y;

    void get_pos();

};


#endif

mouse.cpp:

#include <allegro.h>
#include "types.h"
#include "mouse.h"


void position::get_pos() {

    x = (mouse_x - EDGE) / SQUARE;
    y = (mouse_y - EDGE) / SQUARE;

}

I still have the two #defines for the makecol calls, but apart from that, I think I've incorporated all suggestions.

You should replace those exit(1) calls with throw'ing some exception. You may just call exit(1) in the catch handler anyway but its good practice and there may actually be a time when you can do something about the exception being thrown.

Here is some quick code for using exceptions

Exceptions are nicer than sprinkled calls to exit because then your error handling is centralized.

BTW, macros are evil because they introduce errors that are not readily apparent from looking at the source. Imagine something like

#define foo "abcdef"
int main(){
  int foo = 2;
  ...
}

The error from g++ is

x.c: In function ‘int main()’:
x.c:3: error: expected unqualified-id before string constant

If the #define was in a different header file you would be pretty confused for a while about this error. This almost exact situation happened to me when programming in windows land. Some system header file defined something like 'mode' and caused all sorts of compilation errors when I tried to use it as a variable name.

Just fyi, I try to make the public/protected/private levels in my header files make sense in an orderly manner, as any other programmer should only have to look at the header rather than the accompanying cpp file. In a project I'm working on now, a third party engine has a class that integrates another third party engine and their code kind of looks like this:

Wish I could talk about what the engines do, because they are pretty neat, but I'm under contract.

"The only difference between structs and classes in C++ is that the members of a struct have public visibility by default, and the members of a class have private visibility by default."^[1]

Could you give an example of how to do it with the main.cpp code I have? Your example doesn't make much sense to me, and unfortunately neither do any of the online references I looked up.

I realize that. Doesn't change my expectations or preferences though.

Oh, preference. I thought you were saying that that was the difference.

This seems like allegro's fault then. Identifiers beginning with underscore and capital letter are reserved for compiler implementators.

As to structs/classes question, structs are generally used for POD types that are basically the same as C structs. I also use structs for types that have no private section at all (e.g simple function objects).

Something like this:

P.S Your graphics class is either missing a copy constructor, or it should be declared private. Same goes for operator=.

Several of your classes are missing copy constructors and copy assignment operators. It also looks like some of your classes are questionable in the exception safety department. You're also using dynamically allocated objects a lot when there's no need for them, and fixing that will help a lot with the exception safety.

It's still generally best avoided. As you move to more complex programs where you're using more than just a single library, you will almost always end up with name clashes between libraries regardless of your own code.

void blah() throw (myexception) {
  ...
  throw myexception(); // note its not 'new myexception'
}

One point - you should basically never write exception specifications. You'll probably just want to forget that they event exist. See Herb Sutter's article on the subject

I never created a copy constructor as no objects were ever copied. I just added constructors for the various ways that the object would be created in this program. Is one really needed?

"Good practice" (you asked for it ) would be to either go ahead and write them even if you don't need them right this second (what if you come back to this code in 6 months and start writing something that copies or assigns them? boom) or explicitly declare them as private so that they can't accidentally be used.

    graphic *noughts;
    graphic *crosses;
    graphic *buffer;
    graphic *back;
    graphic *scr;

There is no reason to allocate these dynamically. If you don't do so, your game class wouldn't leak memory, nor should you even worry about implementing the destructor (and copying)...

You can also use the initializer list to call the constructors of the stack instances:

game::game():
    quit(0),
    noughts("nought.bmp"),
    crosses("cross.bmp"),
    buffer(WIDTH, HEIGHT),
    back(WIDTH, HEIGHT),
    scr(screen), //is it a good idea to have screen destroyed by the destructor???
    msg_font(load_font("font.pcx", NULL, NULL))
{
    //...
}

As for the question on copy constructors and assignment operator for graphics, you need to decide whether instances of this should be copyable in the first place (by declaring them private and not implementing them / deriving from a simple class that does this to its copy constructor and operator=, a la boost::noncopyable), or if they should be copyable, whether each copy should get their own bitmap resource, or whether these should be shared between copies (e.g through reference counting, for which you can use shared_ptr<BITMAP>)

------
P.S I have a problem with a script on this thread (line numbering of code boxes)? Does it perhaps take too much time, seeing that there are many code boxes here? (Toggling line numbers on/off won't work when the script is stopped.)

Yes, C++ exception handling fails in a lot of ways and I wouldn't specify exceptions in function types except that in my latest project I was getting segfaults until I added the throw's clause. I have no idea why but I just go with the flow.

(Having programmed in Java for a while where exceptions mostly work I would like to be able to specify the exceptions that can be thrown and have the compiler say something if I messed it up.)

Just a minor issue and not C++-specific:

if ((depth = desktop_color_depth()) != 0)

I'd rather see this one split up; it's overloaded, thus more difficult to read, and most of all, it invites errors involving = and ==.

To put it more bluntly and dogmatic: Never make (active) use of side-effects.

IMO it is more likely that you have something wrong elsewhere and since the error specification would lead to small changes in the binary these might cover up the symptoms (temporarily, another change might make it crash again). I've had a program segfault unless a single bool assignment was commented out (the real cause was elsewhere and the program had been working fine despite the bug for a couple of days).

I saw a bunch of points I wanted to make, but I'll only touch on two.

First off, use of enums for constants. I don't like that idea, because that's not what they're for. What's wrong with const ints again? After all... they are constants.

Second, struct vs class. Thomas is right, and if you move on to more civilized languages (like C#) it goes one step further. In C#, structs are passed by value, whereas classes are passed by reference.

Please note that I did not follow this thread. Nothing's wrong with const int, and nothing's wrong with enum. Both have their uses, and their domains are orthogonal. An enumeration is quite explicit in what it's there for: providing a range of values, noone should care (or have to care) how the respective values are represented "underneath".

Yeah, enums have their uses, but not for setting a constant for screen width or height, IMO.

Asking about good practices, is it a good practice to exceed the char type limit?

For example:

char a = 127;
a += 2; // a will be -127 now right?

Instead of:

char a = 127;
if (a + 2 > 127) a = a + 2 - 256;
else a += 2;

If your first statment would be true how would that second statement ever evaluate to true? Never, bacause -127 is not bigger then 127. You can not detect an overflow with a bigger as sign. That makes no sense.

I always thought that char can hold upto 255 values and is unsigned per default? But I could be wrong about this.

No it's not.

char a = 127;
a += 2; // a will be -127 now right?

It depends on the compiler. Some treat the type char type as being signed, some do not. If char's are signed, then yes it will wrap around, if not, it will now equal 129.

If you want to be certain whether or not a char will be signed or not, use signed char or unsigned char.

Sorry, I would than do this:

int a = 127;
signed char b;
if (a + 2 > 127) b = a + 2 - 256;
else b = a + 2;

Actually, I believe numerical literals are ints by default, which means the char would get promoted to an int before the operation, so it would be 129.

Still, the whole thing is really sketchy and I would avoid the situation altogether. Overflows should be considered an error, not a tool.

Well, if you're using it for text you should just use plain char, so that it's in line with whatever your compiler/project uses. If you want a 1 byte integer, you should explicitly use signed/unsigned char.

I believe they should be a signed integer type which matches the word size of the target platform (which isn't always int - wouldn't want to be too consistent or anything).

Yup. char values outside of 0-127 shouldn't be used for text anyway.

int a = 127;
signed char b;
if (a + 2 > 127) b = a + 2 - 256;
else b = a + 2;

I would just do this:

signed char a;
// ...
if (a < 126) a += 2;
else a = 128 - a;

Would this not protect against the problem faced with the wrap-around?

The reason I did that was when I did noughts("nought.bmp"), I got the following error, which I have no idea how to fix:

mingw32-g++.exe -Wall -fexceptions -O2 -c "C:\Users\LennyLen\Documents\source code\tictac++\main.cpp" -o obj\Release\main.o
mingw32-g++.exe -Wall -fexceptions -O2 -c "C:\Users\LennyLen\Documents\source code\tictac++\game.cpp" -o obj\Release\game.o
C:\Users\LennyLen\Documents\source code\tictac++\game.cpp: In constructor `game::game()':
C:\Users\LennyLen\Documents\source code\tictac++\game.cpp:5: error: no matching function for call to `graphic::graphic()'
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:17: note: candidates are: graphic::graphic(const graphic&)
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:16: note: graphic::graphic(BITMAP*)
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:15: note: graphic::graphic(int, int)
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:14: note: graphic::graphic(const char*)
C:\Users\LennyLen\Documents\source code\tictac++\game.cpp:10: error: no match for call to `(graphic) (const char[11])'

I get the same for all of those declarations if I stop them from being dynamic.

In the end, I removed the graphic class altogether. It didn't really serve any purpose.

The initializer list is something new to me. What benefit does it serve that is better than just assigning values inside the constructor?

It seemed a bit odd to me too, butit was suggested. I've changed them to const ints, even though it makes no real difference except stylistically.

I still need to add the exception handling, but that can wait until tomorrow.

If you do not initialize them in the initializer list, they must have a default constructor that can be called. See below.

Efficiency. Every class member must be constructed before control enters the body of the contructor. So, if you don't use the list you have a default construction plus an assignment.

It's the same logic that leads us in C++ to prefer delaying the declaration of variables as long as possible, so they can initialized on creation.

Not only that. There are certain things that can only be initialized, not assigned later, like classes without default constructor , constant members, reference members and base classes.

I think this should be the final revison of this little project now:

main.cpp:

game.h:

game.cpp:

mouse.h:

mouse.cpp:

#include "mouse.h"


void position::get_pos() {

    x = (mouse_x - EDGE) / SQUARE;
    y = (mouse_y - EDGE) / SQUARE;

}

types.h:

#ifndef TYPES_H
#define TYPES_H


typedef enum { NONE, NOUGHT, CROSS } player_type;  // enumerated constants for the player type


#endif

defines.h:

#ifndef DEFINES_H
#define DEFINES_H


#define WHITE makecol(255, 255, 255)
#define BLACK makecol(0, 0, 0)

const int GRIDSIZE = 3, NUMSQUARES = 9, TEXTLINE1 = 175, TEXTLINE2 = 190, WIDTH = 172, HEIGHT = 232, EDGE = 11, SQUARE = 50;


#endif

And here is all the code/resources with a Windows binary.

Is there an advantage to using a #define for makecol(..., ..., ...)?

If the preprocessor simply replaces your WHITE instances with "makecol(255, 255, 255)", you are just reusing the same thing. I'd just use "const int WHITE = makecol(255, 255, 255);" and avoid any redundant function calls to makecol() if you are just using it for white (or black, or lavender, or puce, ...).

If you're worried that it's trying to set the color before setting the color depth, just don't make it a const and set its value inside your init() function.

It was more that I was trying to avoid any global variables.

And yet Allegro 4 and 5 both use globals all over the place. Once you set the screen resolution, you have SCREEN_W, SCREEN_H, you've got mouse_b, etc. It gives you access to globals all over the place.

I think globals have their place. Sometimes you just don't want to pass some variable around through a dozen function calls to be used in one or two places down the chain: you use a global.

I think #defines have their place. Sometimes you just don't want to write the same bit of text over and over in many places: you use a #define.

edit: And to be honest, the #define vs global variable has nothing to do with C++ explicitly. It's the same for any language that supports both. I'm only interested in in C++ specifics for now.

 CResourceManager::add_color(makecol(255, 255, 255), "white");
 CResourceManager::add_color(makecol(0, 0, 0), "black");

 ..

 col = CResourceManager::get_color("white");

That's the proper way of doing it in OOP.
Not with defines. And not with globals.

Whether you make the methods static or not depends on what you need.

You could go further and define a Resource class and inherit all Resources from that... you have to decide if you want:

a) // two methods for every resource
add_a(type_a, id)
add_b(type_b, id)
type_a a = get_a(id);
type_b a = get_b(id);

or:
b) // having to cast everytime you get a resource
add(resource, id)
type_a a = (type_a)get(id);

Both have their pros and cons.

EDIT:
Obviously, if you want to take the second path, you would have to write wrappers around all allegro c types. Otherwise you can't let them extend the resource class.

col = CResourceManager::get_color("white");
I find this very error-prone. It's going to compile ok and bite you at runtime if you mistake "grey" and "gray", for example.

load_bitmap("sprite.bmp"); might compile ok but will bite you at runtime if the image can't be loaded.

Same goes for any resource once you use a ResourceManager.

You could also have global functions instead of macros:

int white() { return makecol(255, 255, 255); }
int black() { return makecol(0, 0, 0); }

Practically no difference with how the macro works, except takes a bit more typing, but can be put in a namespace etc. The macro itself is benign, except perhaps for naming conflicts that they all can cause.

I just use 0 and -1 for black and white

Except that is less readable, won't work on all colour depths and still doesn't solve the question what you do when you want other colours like green...

Just for good measure, you might want to beef up the try...catch with a few more catches so no exceptions can get past your main routine. std::exception would catch any type derived from the standard exception type. And you can use an ellipsis (...) to catch anything else, and still output some kind of "WTF is this..." message so the user doesn't just have the game outright crash on him. In Intern's Quest, we catch std::invalid_argument, std::range_error, std::logic_error, std::runtime_error, and std::exception, in that order (clearly I didn't think ... was necessary at the time).

It allows for really sweet error handling in the application code. For starters, I just said everything is fatal, and would throw an exception for everything, which were only caught in main. It worked out really great because the game would typically output some kind of error message and exit gracefully for anything (Allegro complicated things because of how I was using timers, etc... Occasionally Allegro timers would not be uninstalled and would fire after Allegro was deinitialized and it would crash anyway, but I think that is mostly cleaned up now).

When you're just catching fatal errors in main, there's really not much point catching anything other than std::exception and any exceptions thrown by 3rd party libs which don't dervive from std::exception. Anything that derives from std::exception should adequately report whatever the problem was via its what() method, so there's no reason to catch derived types unless you're actually going to take some action specific to that type.

In which case you might also derive your own exception type for a particular error (e.g bitmap_load_error). Pick one of the stdexcepts as the base, and you'll only need a constructor that passes the exception message to the base constructor. It is also good to derive them from std exceptions, so you could always catch them with std::exception& if you don't want a particular action.

It's probably better to actually make your own intermediate exception type and then derive from it, because the standard doesn't require std::exception to be able to hold a message (MSVC and some other compilers extend it to do so, GCC doesn't).

I'd also recommend taking a look at Boost.Exception. It's particularly awesome in that you can transport any arbitrary information along with any given exception.

Though the type of exception does carry some information as well, which I found useful to display to the user as well. At least, I found it easier to guess what went wrong when debugging, without having to write type-specific what messages.

IMO any exception should carry enough info to tell you (the programmer) what the problem was. But even if you want to display the name of the exception class, that's done easily enough with typeid(excpetionclass).name().

What I meant is use one of the derivates of the std::exception from <stdexcept> (IIRC std::exception itself doesn't even have a constructor to accept a message). Or naturally you can make your intermediary classes: std::exception > std::runtime_error > my::resource_error > my::bitmap_loading_error.

The type of the exception lets you catch different kinds of exceptions in different places and/or handle them separately. The what message is for a description that you fill in where the exception happens and where you have the exact information. You can also have intermediary try blocks where you catch the message, add more information to it and rethrow it (but I guess that would not be the general pattern and some things might better be left for the debugger).