Rosario Leonardi

  • Content count

  • Joined

  • Last visited

Community Reputation

118 Neutral

About Rosario Leonardi

  • Rank
  1. Code Review - Battleship

    As Indifferent said your code is bad formatted. Organize one class per file is usually a good practice, let you think better about the responsability of each class and about dependency. Use a .h for the declaration of the class and a compile unit for the definition.   After you have subdivided your code in more compile unit is usually more readable. You can use astyle ( to auto-format your code, or eclipse auto-format feature, or CTRL-K CTRL-F if you are using visual studio.   There are some errors/bad practice that is better to avoid - Player class have a virtual table, but don't have a virtual destructor ( You never allocate anything on heap, but it's still a good practice. - Player::go() should be a pure virtual function, it's the interface of player, and is useless if instanced - You forgot to include <cstdlib> there srand is defined. It doesn't compile on gcc otherwise - std::endl is evil. endl flush the output of the stream and it's really slow. Use it only when you really need it. ( - polluting the namespace with unsing namespace std is evil too (   Now let's check the code organization. You have two board, a board is a grid of cell, and even if the data are private you still have total access to the internal representation. There is no logic in this class, only the drawing function and data storage. Note: Use standard name for setter and getter function. Why is called addSpace? It doesn't add anything, it simply set a value on a cell getCellValue setCellValue are better and more standard names.   Ship is.. nothing. It contain only the length and a pointer to board. This class also have the logic to place randomly itself on the board. Well, this is really strange. Why board don't have a function Place ship? Board is a more appropriate place, in this way Ship don't have to know the internal struct of board. Ship have to know that the board is 10 x 10 cell. If you change the size of the board you also have to rewrite the Ship class. Board should contain an array of ship, in this way it can give more information to the user. Is then easy to know how many ship are destroed or give the user a better feedback "Excellent, you destroed the Cruiser, only 3 ship of 5 remaing".   Player is the virtual interface for concrete Human player and AI. Internally have two Board, a misterious X, Y variables and the current score. To be used properly you must call arrangeShip(), if you don't call arrangeShip() the player can't play a proper game.. or better the other player will never hit anything and will never reach 17 point to win. Even better you have to call arranShip only ONCE. If you call arrangeShip more than once you have a lot of ship and the opponent will reach 17 point earlier, and the opponent will win, but you still have ship on your board. Mother of god! As you can see arrangeShip must not be a public function, but must be private and called on the costructor. Or expose a public function resetPlayer that remove all the ship from the board, reset the score and rearrange the ships. Also player suffer for the same problem of Ship, player must know the internal representation of board to know how to place his ships.   Hit have the same problem. Why you didn't put a function of Board called Hit / Fire / Shoot?   if (boad->Fire(x, y)) {   score++;   printf("Hit!\n"); } else {   printf("Whops\n"); } In this case Player must know that Board store character and that use '#' to store ship. :(     Human is a Player that do the following in his gameplay A) read coordinate    why you call it setXY? Yep, it set two attribute called X and Y.. but why? Because the hit function is wrong. go is more readable in this way virtual void go() { int X, Y; readCoordinates(&X, &Y); opponentBoard->Fire(X, Y); } This will remove the orrible protected attributes X and Y. Note that protected attributes are like public for derived classes, so, you don't have any control over them and you can't put any restriction on them. A MadPlayer want to set X to 10000, he can.   B) Update score.   Using magic numbers is just bad, why 17? Because you know that the sum of the possible score is 17. If you want to add a ship how many place of code you want to touch?   Note: ConvertLetter don't use any representation of human, so is not a method of player but a simple external function.     Computer go method have same problems. Need to know the representation of board, and have to read from board where he fired to check where he hit. Considering the Cyclomatic complexity ( of the AI I'm not surprised that it took "a bit longer than I was expecting." But I'm not sure that I want to read it.. sorry. But at least this function is in the right place.   Then came the gameplay and the main loop. You create two board, and two player (I already said that arrangeShip is wrong). Then print a UI and call Go function on current player, check winning condition and than swap sides. This is pretty staighforward, but why you check the winning condition on player? The game is won when the player destroy all the opponent ship. I think that this is more readable. Player* currentPlayer(&human); Player* opponent(&computer); while(1) { currentPlayer->think(); if (currentPlayer->getOpponentBoard()->numberOfRemainingShip() == 0) { printf("Player%s won\n", currentPlayer->getName()); break; } std::swap(currentPlayer, opponent); }   And that's all. Don't forget that in real world you will write program with other people, so don't understimate the power of a solid interface and the good name of a variable. If I found you code, try to use your Player class (without any documentation) I have to read all the code to discover how it work, and this is a huge waste of time/money in production.
  2. C++ pointers, references, and function return values...

    Give reference (or pointer) to object stored on array is usually a bad idea. In your case your class can be deleted and destroy all the object contained in the array. All the object that have taken reference to there object will have pointer to destroed memory and are likely to crash. Another problem can araise if you are more object to the original array. If the array reach it's capacity will reallocate the memory and move all the objects. Again object that have taken the reference will point to dirt memory. Even if you remove an object from the original array you will have problem. When you remove an object from a vector the remaining object on the right are shifted to occupy the remaining hole. In this case if you have given a pointer to and objected that is shifted the pointer now will point to the wrong object or to dirt memory. Your idea will work only if you never touch the array once you give pointer around.   Another aproach is to store pointer.   class someClass { private: std::vector<Thing*> myVector; public: std::vector<Thing*>& getThings(); //simply returns myVector }; But this case is even worse.. I can give the pointer around but who is the owner of the pointer? Who must delete the object once is used?   class someClass { private: std::vector<std::auto_ptr<Thing> > myVector; public: std::vector<std::auto_ptr<Thing> >& getThings(); //simply returns myVector };   auto_ptr solve the problem (shared_ptr if you use the new standard). Smart pointer add refcount of object, so, the last that use the object will automatically delete it. Is still a bad idea tu use standard smart pointer in game development cause they tend to fragment the memory, a better solution is to use boost::intrusive_ptr That have some requirement on the object but it's fast and cache friendly.|