Code Review Request

[Alienguard]

Hello Everyone, I hope you are all doing fine.

I am trying to learn C++ and I am following tutorials in youtube, have got some books and other materials for reading and practicing, etc. I am currently working in a multiplayer tic tact toe game in order to practice my skills and increase my learning, however. Even though I know I have made progress, I am not sure if my code is up to standards which is why I would like someone more experienced than I am to give me a review of my code. I am open to criticism and if you think my code sucks, please don't hesitate to tell me so. I thank you all for helping becoming a better C++ programmer.

Again, thank you and I am sorry for all the inconveniences.

I want to tank ne555 for bringing up github. I feel silly for not linking there, specially when the project has been up since day one.

Here is the link to the github project, note that I only uploaded the CPP and Header files as I am coding this in both a linux machine and a windows machine, so portability is important to me. I tested this on Ubuntu using the QT IDE and it runs, o windows I use Visual Studio.

https://github.com/alienguard140/TIC_TAC_TOE

[ne555]

I suggest you to use this https://github.com/

just a quick look

    Player _thePlayer;
    std::vector<Player> _Players;

¿why is there a `Player' and a collection of `Player'?

int& _xRef = _X; ¿what for?

switch (_currPlayer)
{
case 0:
   _PlayerNameRef = _Players[_currPlayer].GetName();

¿is there a need for the switch?

[Alienguard]

Thank you for your suggestion. I actually feel kind of silly for not thinking about Github... Specially since the project is already uploaded there!

Thank you for giving it a quick look, I will think for a moment and try to analyze these specific points that you bring up. Thank You Again.

[JLBorges]

Three things that caught my eye during a swift perusal of the header files:


1. Be careful with names that contain underscores. For instance,

class GameManager // GameManager.h
{
    // ...
	int _X; // underscore followed by an uppercase letter
	int _Y; // underscore followed by an uppercase letter
	...
};

some identifiers are reserved for use by C ++ implementations and shall not be used otherwise;
no diagnostic is required.

— Each name that contains a double underscore __ or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.
— Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.

2. Make the code const-correct. For instance,

class Player // player.h
{
    // ...
    char GetSymbol() const;
    // ...
};

Use const-ness wherever possible: member functions, variables and (yippee) const_iterators

CppCoreGuidelines https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md

15. Use const proactively.

Summary

const is your friend. Immutable values are easier to understand, track, and reason about, so prefer constants over variables wherever it is sensible and make const your default choice when you define a value. It's safe, it's checked at compile time, and it's integrated with C++'s type system. ...

Discussion

Constants simplify code because you only have to look at where the constant is defined to know its value everywhere. Consider this code:

void Fun( vector<int>& v)
{  //...
   const size_t len = v.size();
   //... 30 more lines ...
}

When seeing len's definition above, you gain instant confidence about len's semantics throughout its scope (assuming the code doesn't cast away const, which it should not do). It's a snapshot of v's length at a specific point. Just by looking up one line of code, you know len's semantics over its whole scope. Without the const, len might be later modified, either directly or through an alias. Best of all, the compiler will help you ensure that this truth remains true.
...

Sutter and Alexandrescu in 'C++ Coding Standards: 101 Rules, Guidelines, and Best Practices'


3. Favour adhering to the rule of zero for non-object-oriented types. For instance,

class Board // board.h
{
	~Board(); // avoid (unless there is a design intent to cater for a non-empty destructor at some point in future)
};

Board::~Board() // board.cpp
{
}

Classes that have custom destructors, copy/move constructors or copy/move assignment operators should deal exclusively with ownership (which follows from the Single Responsibility Principle). Other classes should not have custom destructors, copy/move constructors or copy/move assignment operators.

http://en.cppreference.com/w/cpp/language/rule_of_three

[TheIdeasMan]

@JLBorges

There is an easier to read version here, the links work:

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html

It also has a link to the original, from which the html was produced.

Regards :+)

[JLBorges]

Thank you.

[TheIdeasMan]

With const, there is also constexpr which is a compile time specifier that makes the expression immutable, unlike const which can be cast away.

http://en.cppreference.com/w/cpp/language/constexpr

I gather there are a list of things which are recommended to be used as much as possible:

const

constexpr

auto

brace initialization By that I mean value, list or aggregate initialization
member initialization list in constructor - the OP is already doing this

There are probably more of these. I guess one could point at the CppCoreGuidelines and say: "Do all of that as much as possible". But I am trying to point out some things that might be construed as having a somewhat higher level of effectiveness/ usefulness.

http://en.cppreference.com/w/cpp/language/auto
http://en.cppreference.com/w/cpp/language/initialization

[Alienguard]

@JLBorges, Thank you for being specific and clear and for pointing me to some rules and guidelines, Now I know I have some reading to do before continuing to work on this, After this I will use const more proactively.

@TheIdeasMan Thank you for adding that link to the HTML version of the core guidelines, this really helps me a lot. Also, thank you for being clear about const constepr auto. About brace initialization, I am reading Programming Principles and Practice by Bjarne Stroustrup, I saw something about brace initialization and the constructor. I am still trying to get my mind around some of the concepts, but the book is clear and well written and fun.

I will keep working on it, applying the suggestion I read here as well as trying to follow the guidelines, then I will update my GitHub project.

Thank You Guys! Please, by all means, if you have anything else to add. I will listen.