# Tag Info

15

On the whole this is nicely done for an application made during your first week of C programming. The functions are generally reasonably-sized, the code is comprehensible and the design decisions are understandable given the size and purpose of the application. My remarks mostly center around improving the design to support new features and avoid bugs. The ...

12

__ prefix while not(self.__is_victory()) and not(self.__is_full()): row, col = 5, 5 while not(self.__is_case_empty(row, col)): row,col = self.player1.tick_case() self.__is_case_empty(row, col) print(self.__is_case_empty(row, col)) self.__set_case(row, col, 1) ...

11

Please check it if you can, and give your opinion! Enable more warnings warning: this statement may fall through [-Wimplicit-fallthrough=] unused variables int a, b, i; Spell check funtion Do not re-enter main() Yes, it is possible, yet makes code review and debug a bear and hard to spin this code off into its own TicTacToe() function. Don't do that. ...

11

Try not to using namespace std It is considered a bad practice. This is because namespaces were introduced as a way to avoid name collisions, i.e Multiple objects with the same name. The std namespace is HUGE and has hundreds of common identifiers that can interfere with yours. By doing using namespace std you now have no idea what function is a part of the ...

10

Choosing good identifiers When I see a name like options I think of something a player can choose from like "single player" (i.e., play against the computer) or "multiplayer". A better name would be board because that is what it is. torf? Does it mean "true or false"? Every Boolean is true or false. This does not reflect the ...

9

General Observations I see a lot of good programming practices followed here already, keep up the good work. Most of the functions are small and follow the Single Responsibility Principle. Use Library Functions When You Can The function getl() can be implemented using 2 standard C library functions, fgets and strrchr. The fgets() function inputs an entire ...

9

Avoid using namespace std I cannot emphasize enough on how bad this can be, avoid it now. You might not feel its effect now, but it becomes a headache when the size of the codebase increases. Imagine this, you have a class that defines a method cout, this might seem trivia, who would define a method with cout?. But you should know, there are numerous good ...

7

PEP 8 Overall, your code is very clean. However, there are a few minor PEP-8 deviations: .getname() should be .get_name(), as it is two words. In several places, where a statement continues over multiple lines, the indentation is not PEP-8 compliant, such as: self.board = {1: ' ', 2: ' ', 3: ' ', 4: ' ', 5: ' ', 6: ' ', 7: ' ', 8:...

7

One-based indexing Based on this: int places[10] = {'o', '1', '2', ... "\n\n" , places[1], places[2], ... it seems that you're trying to push a square peg (one-based indexing) through a round hole (zero-based indexing). Try to use zero-based indexing instead. Assuming ASCII This: places[i]-48 assumes that you're using ASCII for the compiler ...

7

Returning by reference ttt_init makes an instance of your TTT_BoardGame but that instance has to be copied when returning it. This cannot be done efficiently. Your struct is so trivially small that no performance impact can be noticeable, but since you're expressly doing this for learning, it's worth pursuing the idiomatic method instead: Either accept a ...

7

Your board dictionary can be simplified from board = {1: ' ', 2: ' ', 3: ' ', 4: ' ', 5: ' ', 6: ' ', 7: ' ', 8: ' ', 9: ' '} to a dict comprehension: board = {n: ' ' for n in range(1, 10)} Your win_game function can be greatly simplified, from def win_game(marker, player_id): """Function to check for winning ...

7

What's done well: Your function names are nice and fairly descriptive. Besides a few things (below), your formatting is good and consistent. You're following proper naming conventions You have doc-strings that give a basic idea of what each function does. What can be improved: You have an odd mix of good names, and poor, single-character names. All your ...

6

Should there be Globals.h? I disagree. Globals.h has a single enum that is only meaningful to your Player class. So why create a new Header? Why can't enum class Players just be in Player.cpp? That is the only file that ever accesses the contents of Players. I believe the best thing to do here is to create an anonymous namespace in Player.cpp and let it ...

5

I guess from efficiency point of view this is fine, even that last line. Efficiency points: You create player variable as list and then keep converting to set over and over again, why not have it as set from beginning? From readability and maintenance point of view, you are having lots of duplicities. Player moves are exactly same except for message and ...

5

Welcome to Code Review! Having a class for player seems over complicated. A simple namedtuple would be enough. The main() function is doing most of the heavy lifting. You can have a Game class, which takes the players list (or individual objects) as init parameters, and then implements the game logic. You can reuse the get_player_input when asking for ...

5

Here are some ideas how you could improve your program which I hope will be helpful. Documentation Your use of docstrings is good to see. If you've never read it before, PEP 257 (Docstring Conventions) is useful so you can follow the norms in Python. Name Mangling I see you've used the double underscore for most methods, e.g. __is_full. This has the effect ...

5

I'll just add some additional points that aren't brought up by the other users. Initialize all your variables in __init__ When looking at a class, you should be able to tell all its attributes by looking in the __init__ method. Here it's not clear why you have self.matrix in some methods until you manually follow through all the calls in the initializer. ...

5

Many aspects have been addressed in the other answers so this answer will focus on a higher-level software engineering consideration but, first, a few nitpicks: algorithmic considerations are important! In particular, avoid doing unnecessary work: when a player plays a move, they only ever touch a single row and a single column and, furthermore, might not ...

5

As you are as you say "brand new" to coding this will be less like a review and more like some pointers and advice.. Some pointers Rather than define the functions as arrow functions within the class verticalScore = (score, col) => { Use the shorter syntax verticalScore(score, col) { There is a technical reason due to the way this is associated ...

4

Here some some gentle suggestions for this particular implementation: This isn't really a "validator" as it doesn't validate anything. Maybe consider "evaluator" or something along those lines. When you receive the Matrix, you can store the value of this.Matrix.GetLength(0) and use it instead of calling it multiples times. Since you know the Matrix is ...

4

Naming & Readability Choosen naming mostly conveys purpose (expressive) and follows conventions (camelCase). Design & Structure Very good design for a learner: TicTacToe represents a game instance currently playing having a board (composition) recording current board state initializes game (registering 2 players, resetting board) checks for ...

4

Small review Break up that long line of code. //prints board void printBoard(){ printf("=============\n| %c | %c | %c |\n-------------\n| %c | %c | %c |\n-------------\n| %c | %c | %c |\n=============\n", boardToChar(0), boardToChar(1), boardToChar(2), boardToChar(3), boardToChar(4), boardToChar(5), boardToChar(6), boardToChar(7), boardToChar(8));...

4

First, congratulations on working code. Recommendations: Simplify your state Simplify your controls Simplify your functions Your state is overly complex, even with no user interaction it is very complex. Every action drastically changes every node in your state. I've updated your code sandbox to show these state changes. You are actually building a data ...

4

I don't use numpy, and I'm getting errors when trying to use it on 3.9, but it seems unnecessary here. If you want to pretty-print a grid, that can be done succinctly using join: def format_grid(grid): return "\n".join(" ".join(row) for row in grid) Which, when you replace all uses of asarray, gives output like: X _ _ _ _ _ _ _ O I ...

4

I would like to mention couple of points. You don't need to use switch cases in enum. It make code over complicated. You can rather use EnumSet and normal if-else condition for it. (updated code added below) Correct the spelling of opposite (form oposite). (updated code added below) Static fields should be initialized in static block, rather than in ...

4

Overview Not much in the way of OO design. Not required but unusual for C++ application. With a bit of abstraction you can make multiple types of player all able to play the game (thus you could have a normal user or a computer plater play the game (see blow)). Design Your grid seems a bit over complex: std::vector<std::vector<std::pair<int, ...

4

I would enable all your warnings (and treat them like errors). Warnings in C++ show logic flaw in your reasoning usally and fixing them will save you some time. The warnings show one fatal flaw (missing return) and one oversight (unused variable). You don't need a copy of GetType() in Human and Robot it already exists in Player. Then you will not need the ...

4

You have already received an excellent review. I would suggest using aliases with using here. Make an alias for std::vector<std::map<int, unsigned char>> using Board = std::vector<std::map<int, unsigned char>>; std::cout<<std::is_same<Board, std::vector<std::map<int, unsigned char>>>::value; // 1 Now you can ...

4

Make system() more portable The reason system("cls") isn't portable is because the command on windows is cls but on most other platforms it is clear. You can check for the OS using certain macros. void clear_screen() { #ifdef _WIN32 // windows system("cls"); #else // macOS and linux system("clear"); #endif //_WIN32 } A ...

4

Upvote, what a nice and simple UML Design! Simplicity is a step to archive scalability, maintainability and ensuring software quality. Concerning the UML model: You must use the composition specifier for the relationship between Board and NoughtsAndCrosses since the instance of Board must be provided so that UB is prevented. The relation between Player and ...

Only top voted, non community-wiki answers of a minimum length are eligible