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:

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 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

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

Quote:

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

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.

Quote:

Things you can initialize and dont need to worry if it fails or not, I usually put on the class constructor, like initialing variables.

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

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

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).

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.

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.

Quote:

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

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).

Quote:

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?

Yeah, you really can't do that with constints 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!)

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.

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.

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.

Quote:

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.

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

Quote:

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.

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

Quote:

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.

Fixed. That was me being lazy.

Quote:

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.

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.

Quote:

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.

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

Quote:

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!)

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.

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:

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).

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.

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.

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.

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.

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.)

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 [www.gotw.ca]

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.)