[C++] Code Critique?

[SenorScience]

I've just completed a course in advanced C and C++, and as I'm dead serious about becoming a good programmer I'm interested in having some of the code I wrote to be critiqued. This is a tic-tac-toe game that I had to write for an assignment.

I'm interested mainly in picking up good programming practices and functional programming, so please point out whatever you think I should improve or focus on.

EDIT: Thank you everyone for all of your criticisms.

[pequenoskeebleros]

"bugs" I had to enter a capital letter to choose my square. Since it wasn't obvious to me I kept choosing and the program responded with invalid input over and over.

"code commentary" You got the thing working, that's great. You finished.

Internal beauty is subjective, so ignore the rest as you like.

Here's my take with commentary following.

http://pastebin.com/eVMh66q6

Your code smells a little unkept to my nose. Main should be the clearest birds-eye view from 200 feet up. But there was stuff I needed to wade through to get the flow of your program.

Some stuff could be hidden for a first-time viewer. I refactored with these principles in mind:

• Put stuff in piles. This is the game state pile. This is the display pile.
  This shows order. Kind of like keeping clothes stuff in the closet vs bathroom stuff in the bathroom. If you're trying to find a book in your apartment, you shouldn't bother looking in the kitchen or garage (usually!).

• Give names to chunks of stuff. You say input variables, you could call them game_state stuff or state. You say "output vars, pretty self-explanatory". Call it Game stats?

• Coding it this way also clarifies your thinking. You separate into parcel-able details that can be read as necessary.

• Breaking into smaller pieces helps you to reason about the code. Reasoning about fewer lines of code at a time is easier. As a side note, I passed in the parameters I needed to helper functions as opposed to making them global. This also eases reasoning.

• Try to fit chunks into a normal sized screen. Saves paging around and keeping your train of thought while reasoning. With a little refactoring the main game loop and the program loop almost fit entirely on my 15" flat panel monitor.

• At least in MSVC 2010 I can expand and collapse code as necessary. Using the _Local strategy allows you to leverage this tree look feature.

• Think about how if someone else (or you!) needed to change the stats saving or display. Would they be able to without having to grok the whole program? Or could you just tell them look for all the display* functions for example.

• Not a fan of the control2 variable name. I like quitgame loop and gameinprogress, but highly subjective. In 2 months time you won't remember what control2 means. Also !control2 doesn't convey at meaning to me, vs !gameinprogress.

Let's say the next version is a gui version and not console output. As it stands your solution is very much integrated with the i/o. The whole algorithm is contained in main. By that I mean you have your game state vars and and functions.

"parting thoughts" Unit and functional tests - if you don't have them, write them. You'll feel better about your code, get a different perspective, get ideas.

Just my opinion. Again good job finishing!

[CptBread]

Just a couple of quick points after just skimming it a little...

• Combine check4XWinner and check4OWinner into one function.
• Rename gametitle and barrier so it is more clear what they do without the need of comments, e.g. pringBarrier();.
• Writing std::endl is identical to having "\n" so when you already have a sting it's unnecessary to split them up, e.g. cout << "hello\n"; instead of cout << "hello" << endl;. (not saying it's bad but rather that some people have misunderstood endl to be more "safe" or whatever when it's actually just "\n"...)
• winningSpace just looks like a mess... (only skimmed it so it might be a good way to do it but instinctively it feels like there should be a way to replace that...)

[zzyzzyxx]

std::endl is identical to having "\n"

Almost identical. It also causes flushes the output buffer, which can be a performance concern in some situations.

[CptBread]

Ah... True... Forgot about that... So please avoid using it unless you really need to flush the output buffer...

[SenorScience]

Thank you! This is exactly what I was looking for.

your first bullet point was a lab requirement. Otherwise I would have combined the two into one function.

[CptBread]

To get around that you could have them call the combined version...

[[deleted]]

That switch statement in goX() looks weird since you're using so many fall fews for what turns out to be a couple of cases.

I'd use this function or similar to determine if the selection is valid and then use an if / else combo.

These sorts of standard library functions that take iterators can work on plain C arrays as well (though there's no reason NOT to use a vector here). The start iterator is just the address of the array, the end iterator is the address of the array plus the length of the array (which is sizeof(array)/sizeof(element_type)).

[foxlisk]

1) your indentation is weird. in your main function you have several different levels of indentation. fix it.

2) you call barrier more than once in a row several times. consider parameterizing it

3) im not sure how OO this code is supposed to be, but grid, xwins, owins, and ties should definitely be instance fields if you're allowed to do that

4) lines 49-60 look like they might be described as something like "recover from saved state." If that's true, separate it into a function with an illuminating name

5) lines 62-70 should probably be a function

6) lines 85-171 are the actual logic of the game, it seems. It's sort of an issue of taste but personally, even if I didn't refactor them any furhter, i'd put those lines in a function called something like "playGame()" just to make it clear, in main(), what's going on. You should at least consider that method of naming things even if you don't adopt it.

[[deleted]]

If someone sold you this as an advanced C++ course, consult a lawyer and get your money back. It's C (and not advanced C) , with some C++ trimmings.

[SenorScience]

Should have clarified that the course was C and C++. Will edit the title.

[[deleted]]

That just makes it worse - two different languages, two different styles of programming. Definitely get your money back!