Looking for constructive criticism

Show 80 post(s) from this thread on one page
Page 1 of 3 123 Last
• 09-07-2012
skyliner
Looking for constructive criticism
This is the final assignment for my introductory c++ class. I decided to continue learning on my own, so I'm gonna start by rewritting my project. I would greatly appreciate any feedback/constructive criticism you guys can provide.

Please, bare in mind; the course did not cover, classes, inheritance or any kind of object oriented programming. In fact, we only learned about data types, operators, basic input/output, control structures, functions, arrays and data structures.
The little I know about any other topic, I learned it indirectly, on my own, by trying to solve the problems the project offered.

Code:

```//  Your final project is to create a game called Sub Hunter. // //  BOARD: //  Is a 10 x 10 grid of cells //  Destroyer: //  Represented by the @ symbol //  Has properties consisting of //  Direction facing (N,S,E,W) //  Depth charges - a count of remaining depth charges //  Strength - How much damage it can take before sinking //  Damage taken - how much damage it has taken from the sub //  Has three action points to spend each round - to be input by the player //  Possible Actions: //  Move Forward - move one square in the direction facing //  Turn Left - make a 90 degree turn to the left in the same square //  Turn Right - make a 90 degree turn to the right in the same square //  Ping - look for a sub. Ping allows the destroyer to "see" if the sub is within two squares of its current location. If the sub is within two squares, the destroyer only knows the direction from its position (not the distance) //  Depth charge - drop a depth charge // //  Sub: //  Represented by the ┴ symbol //  Has properties consisting of //  Direction facing (N,S,E,W) //  Torpedos - a count of remaining torpedos //  Strength - How much damage it can take before sinking //  Damage taken - how much damage it has taken from the destroyer //  Has two action points to spend each round - to be determined by the computer //  Possible Actions: //  Move Forward - move one square in the direction facing //  Turn Left - make a 90 degree turn to the left in the same square //  Turn Right - make a 90 degree turn to the right in the same square //  Scope- look for a destroyer. Scope allows the sub to "see" if the destroyer is within two squares of its current location. If the destroyer is within two squares, the sub knows the direction and distance from its position //  Torpedo - shoot a torpedo at the ship (only the direction it is facing) //  Game Play: //  The user inputs their actions as one turn (e.g. Forward, forward, ping). //  The computer determines the subs actions (Scope, torpedo) //  The program then runs the turns, sub first then destroyer based on the users actions and the computers actions and gives appropriate messages to the user based what happens. //  When the destroyer runs out of depth charges it is considered to be sunk by the sub //  If the sub runs out of torpedos, it can run away (head for the boarder of the map) if it successfully runs away the game is a draw //  Depth charges - When dropped they explode in the square they are in causing full damage to that square and half damage to all the squares around it. //  Torpedos - run straight in the direction the sub is facing for 2 squares. They hit for full damage if the sub is on that line within two square of the launch point - otherwise they miss. //  You should determine damage - limits for the ships and amount caused by the weapons, that make the game playable. // //  Extra Credit ideas - implement more! //  Levels to the game (Easy, Hard etc) //  Directional pings - only work in one direction but give exact information //  Launchable depth charges can go X squares away before exploding //  Homing torpedos //  Smarter sub tactics (run quiet ie. harder to find) //  Depth to the grid (3d array, meaning you also need depth for the charges to explode, minimum depth for scoping etc etc) //  Sound and more. Be creative. ////////////////////////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////////////////////////// // Title: Sub Hunter // Name:  ********* // Class: COP1320 // Due:  06/21 // ------------------------------------------------------------------------------------------- #include <cstdlib> #include <iostream> #include <ctime> using namespace std; struct vessel {  short ammo;     short wepDamage;     short hullIntegrity;     short movePoints;     short position[2];     short heading;     short *destination;     bool  visibility;     char  shape; } sub, destroyer; struct board {  static const short COLUMN = 10;     static const short ROW = 10;     short patrolPoint [4][2]; } map; enum orientation {NORTH, EAST, SOUTH, WEST}; enum patrolP {A, B, C, D}; enum coordinates {X, Y}; // Function prototypes; // -------------------------------; void    initialize(); void    drawMap(); void    attack(vessel); void    aI_turn(); void    humanTurn(); void    checkStatus(); bool    moveSub(std::string action); short  ping(vessel vType, bool verbose = false); short  message(std::string code, bool drawBoard = true); bool gameOver = false;      // modified only in the 'message' function. // FUNCTION DEFINITIONS: //-------------------------; // Initialize(): This function initializes the respective struct members to default values. // ------------------------; // CALLS TO: none // CALLED FROM: main. void initialize () {  srand(time(NULL));     destroyer.ammo = 14;     destroyer.wepDamage = 10;     destroyer.hullIntegrity = 140;     destroyer.movePoints = 3;     destroyer.shape = '@';     destroyer.position[X] = rand()%8 + 1;     destroyer.position[Y] = rand()%8 + 1;     destroyer.heading = NORTH;     sub.ammo = 8;     sub.wepDamage = 20;     sub.hullIntegrity = 80;     sub.movePoints = 2;     sub.shape = 193;                sub.visibility = false;     sub.heading = NORTH;     sub.destination = NULL;     sub.position[X] = rand()%10;     sub.position[Y] = rand()%10;     map.patrolPoint [A][X] = map.ROW * .25;     map.patrolPoint [A][Y] = map.COLUMN * .25;     map.patrolPoint [B][X] = map.ROW * .75;     map.patrolPoint [B][Y] = map.COLUMN * .25;     map.patrolPoint [C][X] = map.ROW * .75;     map.patrolPoint [C][Y] = map.COLUMN * .75;     map.patrolPoint [D][X] = map.ROW * .25;     map.patrolPoint [D][Y] = map.COLUMN * .75; } // ping(): The function will calculate and return the distance between the sub and destroyer as well as tell the AI  whether the // ship is or not within shooting range. // ------------------------; // Parameters: // vType (vessel)  - the type of vessel to perform the ping on (sub or destroyer) // verbose (bool)  - if true, calls 'message();' to print the ping results on the screen. // // CALLS TO: message. // CALLED FROM: humanTurn, aI_turn. short ping (vessel vType, bool verbose) {  short proximity = 0, inRange =-1, subPingRange = 3, destroyerPingRange = 3;     proximity = abs(sub.position[Y] - destroyer.position[Y]) + abs(sub.position[X] - destroyer.position[X]);     if (vType.shape == sub.shape)     {    if (proximity <= destroyerPingRange && verbose)             sub.visibility = true;         else if (verbose)             message ("pingFailed");         return proximity;     }     else if (vType.shape == destroyer.shape)     {  if (proximity <= subPingRange)             return inRange;         else             return 0;     } } // message(): The function displays on the screen all the  messages to the user. Before each message the game board // is redrawn, by calling "drawMap()" to avoid text agglomeration on screen. If 'exit' is passed as an argument, the function will // ask the user whether or not to quit, and it will modify the global 'gameOver' depending on the answer. // ----------------------; // Parameters: // code (string)    - What message to display. // drawMap (bool)    - if true the gameBoard will be drawn before the pertinent message. // CALLS TO:        drawMap. // CALLED FROM: humanTurn, checkStatus, main. short message (string code, bool drawBoard) {  char    exit = 0;     if (drawBoard)         drawMap    ();     if (code == "invalidKey")         std::cout <<"Invalid key, please try again ";     else if (code == "mapEdge")         std::cout <<"You can't move beyond the edge of the map";     else if (code == "subHit")         std::cout << "You Have hit the target!!. Hull Integrity: " <<sub.hullIntegrity;     else if (code == "shipHit")         std::cout <<"You have been hit by the sub. Hull integrity: "<<destroyer.hullIntegrity;     else if (code == "missed")         std::cout <<"You missed !";     else if (code == "noAmmo")         std::cout <<"You have run out of charges. You lose, Game Over !" <<std::endl;     else if (code == "ranAway")         std::cout <<"The Submarine has escaped the waters... Game is a draw";     else if (code == "debug")     {  std::cout << "sub X: " <<sub.position[X] <<" sub Y: " <<sub.position[Y] <<std::endl;         std::cout << "destroyer X: " <<destroyer.position[X] <<" destroyer Y: " <<destroyer.position[Y] <<std::endl;         std::cout << "sub Integrity: " <<sub.hullIntegrity <<" destryer integrity: " <<destroyer.hullIntegrity <<std::endl;         std::cout << "sub ammo: " <<sub.ammo <<" destryer ammo: " <<destroyer.ammo <<std::endl;         std::cout << "sub move points: " <<sub.movePoints <<" destroyer movePoints: " <<destroyer.movePoints <<std::endl;         std::cout <<"sub heading is: " <<sub.heading <<std::endl;         std::cout << "proximity is : " <<ping(sub) <<std::endl;     }     else if (code == "welcome")     {  system ("cls");         std::cout <<"Welcome to SUB_Hunter !!" <<std::endl <<"Your Objective is to locate and destroy "                   <<"an enemy submarine that patrols these waters. To locate the sub you must use your radar "                   <<"to ping its location. If the sub is within a two square radious, its location will be "                   <<"revealed. " <<"To attack the sub you must drop depth charges at it. "                   <<"Depth charges will explode on the square you are causing full damage, and half damage to "                   <<"adjacent squares." "\n" <<std::endl                   <<"To fire depth charges, press 'F', to ping press 'E', to turn left press 'A', to turn "                   <<"right press 'D',to turn back press 'S', to turn front press'Q' to move press 'W'"<<std::endl                   <<"TO READ THIS MESSAGE AGAIN press 'H'";     }     else if (code == "pingFailed")         std::cout <<"Not in range ";     else if (code == "exit")     {  std::cout    <<"Do you want to quit the game ?" <<"\n" <<"Yes(y) No(n)" <<std::endl;         std::cin    >> exit;         exit = tolower(exit);         if (exit == 'y')             gameOver = true;             return 0;     }     else if (code == "g_over")     {  if (sub.hullIntegrity <= 0)             std::cout <<"VICTORY You have annihilated the enemy Submarine !!" <<std::endl;         else             std::cout <<"You have been destroyed by the enemy..." <<std::endl;     }     cin.ignore();     cin.get(); } // drawMap(): This function prints the game board, the ship and the submarine on the screen. The board is a two dimensional // char array of size ROW and COLUMN. The function uses a 'for' loop to iterate through each index of the array and compare // it to the submarine and destroyer's position. These positions are a set of coordinates, (X,Y) representing array indexes. // If the indexes coincide, the ship or sub characters will be stored there. Otherwise the "land" character will be stored. The // contents of each index is printed sequentially on each iteration. // ----------------------; // CALLS TO: none // CALLED FROM: main, message. void drawMap () {  char gameBoard [map.ROW][map.COLUMN];     char land = '|', pit = '_', terrain = land;    // "water" would be more corect since we are dealing with marine vessels :)     char stern = destroyer.shape, bow = '*';     short shpPosX = destroyer.position[X], shpPosY = destroyer.position[Y];     system ("cls");     for (short i=0; i<map.ROW; i++)     {  for (short j=0; j<map.COLUMN; j++)         {    if (sub.visibility == true && j == sub.position[X] && i == sub.position[Y])                 terrain = sub.shape;             else if (j == shpPosX && i == shpPosY )                 terrain = stern;             else if ((destroyer.heading == NORTH && i == shpPosY - 1 && j == shpPosX) ||                         (destroyer.heading == SOUTH && i == shpPosY + 1 && j == shpPosX) ||                         (destroyer.heading == EAST && i == shpPosY && j == shpPosX + 1) ||                         (destroyer.heading == WEST && i == shpPosY && j == shpPosX - 1))                 terrain = bow;             else if (j != shpPosX || i != shpPosY)                 terrain = land;             gameBoard[j][i] = terrain;             std::cout <<gameBoard[j][i] <<pit;         }         std::cout <<std::endl;     } } // humanTurn(): The function controls the user interactions, takes and validates user inputs, deducts movement points and // moves the destroyer, providing feedback to the user along teh way. // ----------------------; // CALLS TO: message, atack, ping. // CALLED FROM: main. void humanTurn () {  char  key = 0;     short nextPosX    = destroyer.position[X], nextPosY = destroyer.position[Y];     while (destroyer.hullIntegrity  && destroyer.movePoints)     {  std::cout    <<"Your move " <<"(" <<destroyer.movePoints <<")" <<" moves left ";         std::cout  <<destroyer.ammo <<" charges left" <<std::endl;         std::cin    >>key;         key = tolower(key);         if (key == 'w' || key == 'd' || key == 's' || key == 'a' || key == 'q'|| key == 'e' || key == 'f' || key == 'b' || key =='x' || key == 'h')             break;         else             message ("invalidKey");     }     if (key == 'q')         destroyer.heading = NORTH;     else if (key == 'd')         destroyer.heading = EAST;     else if (key == 's')         destroyer.heading = SOUTH;     else if (key == 'a')         destroyer.heading = WEST;     else if (key == 'w' )     {  if (destroyer.heading == NORTH)             nextPosY --;         else if (destroyer.heading == EAST)             nextPosX ++;         else if (destroyer.heading == SOUTH)             nextPosY ++;         else if (destroyer.heading == WEST)             nextPosX --;     }     else if (key == 'e')         ping (sub,true);     else if (key == 'f')         attack (sub);     else if (key == 'b')         message ("debug");     else if (key == 'x')         message ("exit");     else if (key == 'h')     {  message ("welcome",false);         key = 0;     }       if (nextPosX < 0 || nextPosX > map.ROW-1 || nextPosY < 0 || nextPosY > map.COLUMN-1)           message ("mapEdge");     else if (key)     {  destroyer.position[X] = nextPosX;         destroyer.position[Y] = nextPosY;         destroyer.movePoints --;     } } // attack(): This function determines if the destroyer or sub are within shooting range, if so it deducts ammo points and health // from whatever is specified as vessel type and It informs the user of its attack results as well as the sub's. // ----------------------; // Parameters: // vType (vessel)  - what vessel to attack, sub or destroyer. // CALLS TO: message, ping, moveSub. // CALLED FROM: void attack (vessel vtype) {  short proximity = 0;     if (vtype.shape == sub.shape)     {  proximity = ping(sub);       if ( proximity < 2)       {  sub.hullIntegrity -= (destroyer.wepDamage / (proximity + 1));           sub.visibility = true;           message ("subHit");       }       else           message ("missed");       destroyer.ammo --;     }     else if (vtype.shape == destroyer.shape)     {  if (moveSub("chase") == true);         {  while (sub.movePoints > 0 && sub.ammo && destroyer.hullIntegrity)             {  sub.ammo --;                 sub.movePoints --;                 destroyer.hullIntegrity -= sub.wepDamage;                 message ("shipHit");             }         }     } } // moveSub(): The function is in charge of controlling the sub movements. Depending on the arguments passed, it will Patrol a // predefined area around the map, move the sub away from the destroyer, drive the sub away from the map, when the sub // runs out of charges, or it will position the sub at the correct firing distance and  the correct heading to discharge torpedos at // the destroyer. // ----------------------; // Parameters: // action (string)  - will tell the function what action to perform, whether to chase the destroyer, evade, run away from the map // or patrol the area. // CALLS TO: none // CALED FROM: attack, aI_turn. bool moveSub (string action) {  short distanceX = 0, distanceY = 0, absDstX = 0, absDstY = 0;     // If "chase" is passed as an argument, the function will find the shortest path to the destroyer by finding the difference     // in the X and Y positions between both vessels. It will then move the sub in that axis, towards the destroyer, by increasing     // its position if the difference is negative and decreasing it if positive.     // Everytime the sub position is updated, its orientation is checked, so that it moves in the direction it's heading, if it isn't its     // heading is updated and a move point is deducted.     // If the sub is within fire distance of the destroyer, i.e the absolute value of the difference of their array index position, is     // less or equal to fireRange's value, the function will return true. Orientation will be checked again.     if (action == "chase")     {  short fireRange = 2;         bool inFireRange = true;         while  (sub.movePoints > 0)         {  distanceX = sub.position[X] - destroyer.position[X];        //distance between the sub and the ship in the X axis.             distanceY = sub.position[Y] - destroyer.position[Y];        //distance between the sub and the ship in the Y axis.             absDstX = abs(distanceX);             absDstY = abs(distanceY);             // if the sub is within two squares on the same axis.             if ((absDstX <= fireRange && !absDstY) || (absDstY <= fireRange && !absDstX))             {  if (distanceX < 0 && sub.heading != EAST)                 {  sub.heading = EAST;                     sub.movePoints--;                 }                 else if (distanceX > 0 && sub.heading != WEST)                 {  sub.heading = WEST;                     sub.movePoints--;                 }                 else if (distanceY < 0 && sub.heading != SOUTH)                 {  sub.heading = SOUTH;                     sub.movePoints--;                 }                 else if (distanceY > 0 && sub.heading != NORTH)                 {  sub.heading = NORTH;                     sub.movePoints--;                 }                 return inFireRange;             }             else // if the path is closer on the X axis             {  if (absDstX < absDstY && absDstX || !absDstY)                 {  if (distanceX < 0)                     {  sub.position[X] ++;                         if (sub.heading != EAST)                         {  sub.heading = EAST;                             sub.movePoints--;                         }                     }                     else                     {  sub.position[X] --;                         if (sub.heading != WEST)                         {  sub.heading = WEST;                             sub.movePoints--;                         }                     }                 }                 else                 {  if (distanceY < 0)                     {  sub.position[Y] ++;                         if (sub.heading != SOUTH)                         {  sub.heading = SOUTH;                             sub.movePoints--;                         }                     }                     else                     {  sub.position[Y] --;                         if (sub.heading != NORTH)                         {  sub.heading = NORTH;                             sub.movePoints--;                         }                     }                 }                 sub.movePoints --;             }         }         return false;     }     // If "patrol" is passed as an argument, the sub will begin to move towards predefined set-points. First  the sub's position is     // checked. if its position does not coincide with one of the patrol points then point "A" is set as the default destination.     // Once it arrives there, point "B" is set as the next destination, and then "C".. This goes on and on until the sub moves     // through all the points, and then begins again.     // 'Destination' is a pointer variable that will hold the address of the patrol point the sub is heading to. The X and Y position     // of the destination point is then extracted from 'destination' and substracted with the sub's X and Y positions. The sub     // will then increase or decrease its position based on whether the difference is positive or negative by an amount     // corresponding to the absolute value of that difference.     else if (action == "patrol")     {  short moveRow = 0, moveColumn = 0, posX = 0, posY = 0;         short *destinationY = NULL;         if (sub.position[X] == map.patrolPoint[A][X] && sub.position[Y] == map.patrolPoint[A][Y])             sub.destination = &map.patrolPoint[B][X];         else if (sub.position[X] == map.patrolPoint[B][X] && sub.position[Y] == map.patrolPoint[B][Y])             sub.destination = &map.patrolPoint[C][X];         else if (sub.position[X] == map.patrolPoint[C][X] && sub.position[Y] == map.patrolPoint[C][Y])             sub.destination = &map.patrolPoint[D][X];         else if (sub.position[X] == map.patrolPoint[D][X] && sub.position[Y] == map.patrolPoint[D][Y])             sub.destination = &map.patrolPoint[A][X];         else if (!sub.destination)             sub.destination = &map.patrolPoint[A][X];         posX = *sub.destination;         distanceX = sub.position[X] - posX;         moveRow = abs(distanceX);         destinationY = &sub.destination[Y];         posY = *destinationY;         distanceY = sub.position[Y] - posY;         moveColumn = abs(distanceY);         for (short i = 0, j = sub.movePoints; j > 0 && i < moveRow; i++, j--)         {  if (distanceX > 0)                 sub.position[X]--;             else                 sub.position[X]++;         }         for (short i = 0, j = sub.movePoints; j > 0 && i < moveColumn; i++, j--)         {  if (distanceY > 0)                 sub.position[Y]--;             else                 sub.position[Y]++;         }         sub.movePoints = 0;     }     // if evade is passed as an argument, the sub will move backwards, in the opposite direction it's heading. This moves the sub     // directly away from below the destroyer, where it causes more damage and saves movepoints to fire at least one shot in     // that turn, by not having to waste points turning. "evade" is passed when "aI_turn" determines the sub is low on health.     else if (action == "evade")     {  if (sub.heading == NORTH && sub.position[Y] < map.COLUMN)             sub.position[Y]++;         else if (sub.heading == SOUTH && sub.position[Y] > 0)             sub.position[Y]--;         else if (sub.heading == EAST && sub.position[X] > 0)             sub.position[X]--;         else if (sub.heading == WEST && sub.position[X] < map.ROW)             sub.position[X]++;         sub.movePoints--;     }     // If "runAway" is passed as an argument, the function will move the sub out of the map, finding the fastest route by     // dividing the size of the array in 2 and comparing the current sub position with it.     // "aI_turn" will pass this argument if the sub runs out of torpedos. This functionality was part of the assignment but the     // game mechanics do not allow for it to be used, since the sub will kill the destroyer before running out of charges.     // Reducing the number of torpedos would make the destroyer not-killable so that isn't an option. The only option would be to     // make the sub fire "test" shots every now and then, but since the torpedo range is the same as the ping range, theres not     // any advantage to this. A more attractive option would be to make a shipwright base where the destroyer can get repairs     // provided that the sub doesn't find it first.     else if (action == "runAway")     {  short middle = 0;         distanceX = sub.position[X] - map.ROW;         distanceY = sub.position[Y] - map.COLUMN;         absDstX = abs(distanceX);         absDstY = abs(distanceY);         middle = map.ROW / 2;         if (absDstX < absDstY)         {  if (absDstX <= middle)                 sub.position[X]++;             else                 sub.position[X]--;         }         else         {  if (absDstY <= middle)                 sub.position[Y]++;             else                 sub.position[Y]--;         }         sub.movePoints--;     } } // This function controls all the AI decisions and behavior. if the sub is directly below the destroyer with low health it will pass // "evade" to moveSub. If the destroyer is discovered, it will call the "attack" function. Otherwise it will pass "patrol" to moveSub. // If the sub runs out of ammo, it will pass "runAway"(note* see "runAway" section on the moveSub function to see why this will // never occur. // ------------------------; // CALLS TO: moveSub, ping. // CALLED FROM: main. void aI_turn() {  if (destroyer.movePoints <= 0)     {  sub.visibility = false;         if (sub.ammo)         {  if (sub.position[X] == destroyer.position[X] && sub.position[Y] == destroyer.position[Y] &&                 sub.hullIntegrity <= destroyer.wepDamage * 3 && destroyer.hullIntegrity > sub.wepDamage * 2)                 moveSub("evade");             else if (ping(destroyer))               attack (destroyer);             else                 moveSub("patrol");         }         else             moveSub ("runAway");     } } // checkStatus (): This function will check the game looking for game ending conditions. If found, a propper message will be // issued to the user and the game will finish. After both sub and destroyer have played out their turns their move points are // restored. // ------------------------; // CALLS TO: message, initialize. // CALLED FROM: main. void checkStatus() {  if (!gameOver)     {  bool game_over = 0;         if (sub.hullIntegrity <= 0 || destroyer.hullIntegrity <= 0)         {  message ("g_over");             game_over = true;         }         else if (destroyer.ammo <= 0)         {  message ("noAmmo");             game_over = true;         }         else if (sub.ammo <= 0 && (sub.position[X] >= map.ROW || sub.position[Y] >= map.COLUMN))         {  message ("ranAway");             game_over = true;         }         if (game_over)         {  message ("exit");             initialize();         }         else if (!sub.movePoints  && !destroyer.movePoints)         {  destroyer.movePoints = 3;             sub.movePoints = 2;         }     } } int main(int argc, char *argv[]) {     message("welcome",false);     initialize();     while (!gameOver)     {    drawMap();         aI_turn();         humanTurn();         checkStatus();     }     return 0; }```

I hope this also can help future students, scavenging the web for answers, to get a general idea of how this can be approached, and avoid being as lost as I was when I first started. Word of caution though, the professor sends all submitted code through a code analyzer that compares your code with a database of previous submissions. Any similarity higher than 60% will raise red flags all over and you will most likely get an F. So take the concepts and implement them in your own way.
• 09-07-2012
jimblumberg
Quote:

main.cpp||In function ‘void initialize()’:|
main.cpp|128|warning: overflow in implicit constant conversion [-Woverflow]|
main.cpp||In function ‘void attack(vessel)’:|
main.cpp|365|warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body]|
main.cpp||In function ‘bool moveSub(std::string)’:|
main.cpp|429|warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]|
main.cpp|627|warning: unused parameter ‘argc’ [-Wunused-parameter]|
main.cpp|627|warning: unused parameter ‘argv’ [-Wunused-parameter]|
main.cpp||In function ‘bool moveSub(std::string)’:|
main.cpp|563|warning: control reaches end of non-void function [-Wreturn-type]|
main.cpp||In function ‘short int message(std::string, bool)’:|
main.cpp|244|warning: control reaches end of non-void function [-Wreturn-type]|
main.cpp||In function ‘short int ping(vessel, bool)’:|
main.cpp|173|warning: control reaches end of non-void function [-Wreturn-type]|
||=== Build finished: 0 errors, 8 warnings ===|
Some of the warnings may not be too important but can easily be fixed (control reaches end of non-void function). For the first warning:
Quote:

main.cpp|128|warning: overflow in implicit constant conversion [-Woverflow]|
A signed character can only hold values between -127 to 127 so a value of 193 is too large for this data type.
Quote:

main.cpp||In function ‘void attack(vessel)’:|
main.cpp|365|warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body]|
This warning message is telling you there is something wrong with your if statement on line 365 and you probably want to look into that problem because it shouldn't be an empty body.

Quote:

main.cpp|429|warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]|
I suggest you use parentheses when mixing AND and OR operators to insure you are properly evaluating these statements in the proper order. I don't recommend you rely on operator precedence as the proper parentheses make your intent much clearer.

Also I'm not too fond of using the global variables for your structure instances. I would make them local to a function and pass them to the functions as required.

Also I would consider wrapping some of your long lines to make reading your program a bit easier.
Code:

```if (sub.position[X] == destroyer.position[X] &&         sub.position[Y] == destroyer.position[Y] &&         sub.hullIntegrity <= destroyer.wepDamage * 3 &&         destroyer.hullIntegrity > sub.wepDamage * 2)   moveSub("evade");```
But I will say your indentation is very consistent which does help following your logic.

You may also want to consider breaking up your moveSub() function into smaller sub functions.

Overall I would say for an introductory level class this program seems well done.

Jim
• 09-07-2012
Elysia
Problem is that whether char is signed or unsigned by default is implementation defined AFAIK.
So to get around it, if you want to use char, is to explicitly use unsigned char.

Aside from the global variables, just scanning through the code, if you have C++11 support in your compiler, consider replacing C-arrays with std::array and NULL with nullptr.
I also see inconsistencies. You use "using namespace std;", yet you explicitly refer to, for example, cout as std::cout. Why? Is there a reason you are using "using namespace std;" if you want to explicitly qualify the namespaces?
In your functions that take a string, consider making them into an enum and using a case. It's much more efficient and faster.
• 09-07-2012
oogabooga
This is a very C-ish C++ program.
Why are you are using globals?.

Consider using a switch statement in humanTurn().
Code:

```    bool again = true;     while (again) {         again = false;         std::cout << ...;         std::cin >> key;         switch (tolower(key)) {         case 'q':             ...;             break;         case 'd':             ...;             break;         default:             again = true;             message("invalidKey");         }     }```
moveSub is too big and is really 4 completely separate functions.
Instead of calling, for example, moveSub("runAway"), consider subRunAway() (or in a more c++-y program, sub.runAway() ).

Code:

`short vessel::position[2]`
consider
Code:

```struct point {     int x, y; }; point vessel::position;```
Code:

```enum element names too short: A B C D X Y short board::patrolPoint[4][2]```
consider
Code:

```struct board {  static const short COLUMN = 10;     static const short ROW = 10;     struct {         point A, B, C, D; //or NW, SW, SE, NE;     } patrolPoint; } map; map.patrolPoint.A.x = ...;```
I'm not sure if a function like message() is "good" from a computer-science perspective. It's kind of a message clearing house. I'd be interested in other's opinions on this.
• 09-07-2012
Elysia
Quote:

Originally Posted by oogabooga

Because his/her class has not covered classes yet.
• 09-07-2012
Elkvis
I think it's very unfortunate that the introductory class didn't touch on classes and inheritance at all. It seems like this program would have been a perfect fit for those concepts.
• 09-07-2012
Elysia
Indeed. A rather backwards way of teaching these days, I'd say.
• 09-07-2012
grumpy
I don't agree that classes and inheritance should be taught early in a C++ class, or that failing to do so is "backwards".

In this example, I'm more concerned about the use of static variables, and the fact of duplication of low-level operations that could be moved to separate functions. The usage of enums as array indices is a really bad habit to get into. Rather than using lots of if/else blocks testing strings, I'd look into using arrays (or maps), and using some algorithm (or simply a loop construct) to do the heavy lifting. This code is therefore a lot more error prone (hard to get right) than it needs to be. If you want to clear the screen, there are better ways than system("cls") .... albeit all of them just as non-portable as system("cls").
• 09-08-2012
skyliner
Thank you all for your replies.

Quote:

Originally Posted by jimblumberg

That is a VERY VERY useful compiler you have. I got none of such warnings,
Quote:

A signed character can only hold values between -127 to 127 so a value of 193 is too large for this data type.
The professor specifically asked for the ┴ symbol to be used. According to the ASCII table, the numeric value for that symbol is 193. What you say however would explain why the symbol was never properly displayed in my machine. (Though it did on windows.)
Quote:

This warning message is telling you there is something wrong with your if statement on line 365 and you probably want to look into that problem because it shouldn't be an empty body.
Again that is a NICE catch. I am loving that compiler.
Indeed that is a mistake, and one that brought me many headaches.

Quote:

I suggest you use parentheses when mixing AND and OR operators to insure you are properly evaluating these statements in the proper order. I don't recommend you rely on operator precedence as the proper parentheses make your intent much clearer.
I will do that from now on. Thanks.
Quote:

Also I'm not too fond of using the global variables for your structure instances. I would make them local to a function and pass them to the functions as required.
Do you mean all the structure members or just the static constants in "board"
Quote:

Also I would consider wrapping some of your long lines to make reading your program a bit easier.
Code:

```if (sub.position[X] == destroyer.position[X] &&         sub.position[Y] == destroyer.position[Y] &&         sub.hullIntegrity <= destroyer.wepDamage * 3 &&         destroyer.hullIntegrity > sub.wepDamage * 2)   moveSub("evade");```

Splitting statements like that is one of the things I learned on my own. Rather late into the project. I went back and used it on drawMap(), but I guess I missed
a few other long lines.
Quote:

You may also want to consider breaking up your moveSub() function into smaller sub functions.
That is the first thing I'm doing in my rewrite.
Quote:

Overall I would say for an introductory level class this program seems well done.
Jim
Thank you.

Quote:

Originally Posted by Elysia
Problem is that whether char is signed or unsigned by default is implementation defined AFAIK.
So to get around it, if you want to use char, is to explicitly use unsigned char.

Aside from the global variables, just scanning through the code, if you have C++11 support in your compiler, consider replacing C-arrays with std::array and NULL with nullptr.
I also see inconsistencies. You use "using namespace std;", yet you explicitly refer to, for example, cout as std::cout. Why? Is there a reason you are using "using namespace std; if you want to explicitly qualify the namespaces?
In your functions that take a string, consider making them into an enum and using a case. It's much more efficient and faster"

I have no idea, what the using namespace std" statement does. I always thought it had to do with "standard spaces" ???. In any case when I started my project in linux, the starting console project had this "std::cout <<"hello world" <<std::endl;" line, and I noted that a simple cout, wouldn't work. So I started using "std::"

Not long after starting, I switched over to DevC++, cause the professor said we should use that and only that IDE to avoid unnecessary problems. The starting console project in DevC++ did have the "using namespace std" line. I never put much thought into it, since the previous code worked fine, so I kept on using std::.

Quote:

Originally Posted by oogabooga
This is a very C-ish C++ program.
Why are you are using globals?.

The course did not cover classes. We couldn't use them and if we did we had to got to the professor's office and demonstrate our knowledge of classes in front of him.
I know i shouldn't use globals, but I used just one, and specified the only place where it is modified. isn't that what is hold against global vars - that they can be modified everywhere in the program- ?

We used shorts in most of the programs in class. Shorts occupy only 2 bytes in memory, instead of the 4, integers take. And the numbers used are so small, that they fit more than comfortably in a short var.
Quote:

Consider using a switch statement in humanTurn().
Code:

```    bool again = true;     while (again) {         again = false;         std::cout << ...;         std::cin >> key;         switch (tolower(key)) {         case 'q':             ...;             break;         case 'd':             ...;             break;         default:             again = true;             message("invalidKey");         }     }```

Could you please explain why thats better than my solution.
Quote:

moveSub is too big and is really 4 completely separate functions.
Instead of calling, for example, moveSub("runAway"), consider subRunAway() (or in a more c++-y program, sub.runAway() ).
I agree. I am going to split it in my rewrite. I like this part very much:
Code:

`sub.runAway()`
i wasn't aware I could make functions inside of structures. I'll look into it.

Quote:

Code:

`short vessel::position[2]`

I do not understand what this statement does. I assume it creates a short array, and makes it a member of the vessel structure ?
I'm not familiar with the operator "::"
Quote:

Code:

```struct point {     int x, y; }; point vessel::position;```

Are you making a structure a member of another structure. ?
That is some clever use of structures. I've never seen that before.
Quote:

Code:

```enum element names too short: A B C D X Y short board::patrolPoint[4][2]```
consider
Code:

```struct board {  static const short COLUMN = 10;     static const short ROW = 10;     struct {         point A, B, C, D; //or NW, SW, SE, NE;     } patrolPoint; } map; map.patrolPoint.A.x = ...;```

You are creating a structure inside of another structure, again I've never seen that before.That's a very clever way to do it :)

Quote:

Originally Posted by grumpy
In this example, I'm more concerned about the use of static variables, and the fact of duplication of low-level operations that could be moved to separate functions.

I'm not too happy with that duplication of code either. I'm gonna see how can I make it into functions.
Quote:

Rather than using lots of if/else blocks testing strings, I'd look into using arrays (or maps), and using some algorithm (or simply a loop construct) to do the heavy lifting
Are you suggesting to use an array to store the message strings, and then comparing the indexes in a loop.?
That would save me the time when checking, but I would still have to assign the string to each array index manually. Unless I use numbers instead of strings, as arguments for message, in which case the program would lose readability.
message("gameOver") is more meaningful that message(7).
Quote:

The usage of enums as array indices is a really bad habit to get into
Would you please explain why, and show me an alternative way to do it.
• 09-08-2012
grumpy
Quote:

Originally Posted by skyliner
That is a VERY VERY useful compiler you have. I got none of such warnings,

You need to read the documentation for your compiler. Compilers "out of the box" often do not give detailed warnings or diagnostics. However, there are ways to configure most modern compilers so they do give detailed diagnostics.

It is generally considered good practice to both use your compiler in a way that it gives as many warnings or diagnostics as possible, and to write your code well enough that the compiler does not complain.

Unfortunately, compiler vendors typically disable detailed diagnostics, because - historically - a lot of lazy programmers preferred not to see them, and complained loudly.

Quote:

Originally Posted by skyliner
Do you mean all the structure members or just the static constants in "board"

I mean the variables that you declare at file scope. I do not mean the type definitions (of structs, enums, etc).

For example, you have
Code:

```struct vessel {  short ammo;     short wepDamage;     short hullIntegrity;     short movePoints;     short position[2];     short heading;     short *destination;     bool  visibility;     char  shape; } sub, destroyer;```
This code both specifies a struct type named vessel, and two instances of vessel named sub and destroyer. Defining a type at file scope is a good idea as it allows the type to be reused. Defining instances is not. So I would find a way to tease out the type definition
Code:

```struct vessel {  short ammo;     short wepDamage;     short hullIntegrity;     short movePoints;     short position[2];     short heading;     short *destination;     bool  visibility;     char  shape; };```
and then create sub and destroyer within some function, and pass them as arguments to functions, rather than simply having their names globally available to all functions.

Note also, that if you are passing larger structs to a function, it is often practically a good idea to pass them by reference rather than by value.
Code:

```void do_something(vessel &sub) {     // do things to the submarine, that may or may not change its state } void do_something_else(const vessel &sub) {     // do things to members of sub that do not change the sub }```
Quote:

Originally Posted by skyliner
I have no idea, what the using namespace std" statement does. I always thought it had to do with "standard spaces" ???. In any case when I started my project in linux, the starting console project had this "std::cout <<"hello world" <<std::endl;" line, and I noted that a simple cout, wouldn't work. So I started using "std::"

A namespace is, in mathematical lingo, a space containing names. std is the name of a namespace. std::cout is an object named cout, within namespace std. If you had another namespace, say X, it could also contain something named cout. X's cout is distinct from std's cout, and is accesses as X::cout.

A "using namespace std;" tells the compiler to treat all names within the std namespace as candidates later on. After a "using namespace std;", the statement "cout << x;" will view std::cout as a candidate match for "cout" (assuming <iostream> has also been #include'd).

The problem with "using namespace" is that they cause ambiguity. Let's say you have
Code:

```#include <iostream> //  declarations of things in namespace X namespace X {     int cout; }; using namespace std; using namespace X; int main() {     cout << "Hello\n"; }```
In this case both std::cout and X::cout are candidates to match "cout". The compiler has no way of knowing which, so will complain about ambiguity. To resolve that, you either need to remove one or both "using namespace" directives, or use the full name (say std::cout) that you intend.

Generally, it is considered a good idea to either use using directives, or fully qualify your names. But you can do both.
• 09-08-2012
whiteflags
Quote:

I have no idea, what the using namespace std" statement does.
When you use namespaces you have to resolve the name to use any of the stuff inside. This is why you see foo::bar in places.

The using namespace directive assists the programmer: where you type the name of something, when the compiler looks for a definition during the compile process, it will consider that namespace. It works for any degree of scope (file, function, block, etc). That is precisely why it is used by newbies the way it is. Everything you need typically is in std, so that gets exposed for entire files.

You can also expose a single function or class from a namespace with a using directive.
Code:

``` using std::string; using std::fabs;```
Both things come with many caveats:
0. Many times, in many places, "using namespace ..." is a bad idea because two names will be used in two different namespaces.
1. "using namespace ..." is a bad idea in header files since it will apply wherever the header is included, which will eventually collide.
2. You will probably collide with something with the same name if you don't know what you're doing. Everyone has a story about sometime this happened to them.
...
Mine was Rectangle.

So find the golden middle and be happy.
• 09-08-2012
jimblumberg
Quote:

Originally Posted by skyliner
That is a VERY VERY useful compiler you have. I got none of such warnings,

If you are using Dev-C++ you are probably using the same basic compiler (although an earlier version). The difference is I am compiling with more warnings enabled. I suggest you read your compiler manual to determine how to enable more warnings.
Quote:

Originally Posted by skyliner
The professor specifically asked for the ┴ symbol to be used. According to the ASCII table, the numeric value for that symbol is 193. What you say however would explain why the symbol was never properly displayed in my machine. (Though it did on windows.)

There are a couple of problems here. First in order to use the "characters" above 127 you must use an unsigned char for the data type. Second these extended characters are not always available, because there were many different extended character sets. For example look at the differences between these characters in the following two links: ASCII TABLE and ASCII CODE and ASCII Comparison. So as you can see with one character set you get one set of extended characters while with the other character set you get something different.

Quote:

Originally Posted by skyliner
We used shorts in most of the programs in class. Shorts occupy only 2 bytes in memory, instead of the 4, integers take. And the numbers used are so small, that they fit more than comfortably in a short var.

Actually the C++ standard doesn't guarantee this. A short and an int are both only guaranteed to hold a value of 32767. Today unless you are programming for a severely memory constrained system I recommend you use the int. The int is usually considered the optimum type for the processor.

Jim
• 09-08-2012
Elysia
Quote:

Originally Posted by skyliner
The professor specifically asked for the ┴ symbol to be used. According to the ASCII table, the numeric value for that symbol is 193. What you say however would explain why the symbol was never properly displayed in my machine. (Though it did on windows.)

OK, so first, you can just type '┴', not 193. Makes it easier to read, don't you think?
Second, the problem with extended characters. They are not standard. There are a lot of so called extended code pages which define these characters above the first 128. It is likely that Linux and Windows does not use the same codepage. Therefore, what shows up on Linux using these extended character sets may show up wrong on Windows and vice versa.
So what do you do about it? You use Unicode. For starters, read this: The Absolute Minimum Every Software Developer Absolutely, Positively Must Know About Unicode and Character Sets (No Excuses!) - Joel on Software
If you can't use Unicode, then complain to your Professor that you shouldn't use it. If that doesn't work, then drop the course. If you can't do that, then you will just have to accept what problems this brings. Just be aware of why it is happening, why you should never use extended character sets and why your Professor is stupid unless he/she has a very good excuse for teaching it. Don't use it in real world programs. Use Unicode, like the rest of the modern world.

Quote:

Not long after starting, I switched over to DevC++, cause the professor said we should use that and only that IDE to avoid unnecessary problems. The starting console project in DevC++ did have the "using namespace std" line. I never put much thought into it, since the previous code worked fine, so I kept on using std::.
Don't use Dev-C++. It's old and unmaintained. If your Professor recommends you to use it, ignore him/her. If your Professor requires you to use it, then complain. If that doesn't work, drop the course. If you can't do that, then use, but know that whenever you possible can, you should drop it and get a modern IDE. Here is a small list of modern IDEs, just for reference: SourceForge.net: Integrated Development Environment - cpwiki

Quote:

I know i shouldn't use globals, but I used just one, and specified the only place where it is modified. isn't that what is hold against global vars - that they can be modified everywhere in the program- ?
There are mainly two problems:
- Your program modifies these variables from different functions, making the flow of the program difficult to ascertain, making optimizations very difficult, and making it difficult to verify code correctness.
- Your program have the ability (but does not necessarily do it) the variable from multiple functions. This opens bugs which the compiler cannot catch because the variable can be modified from every function, but should not necessarily be. Some function may inadvertently modify a global variable when you did not intend do, leading to a possibly difficult to track bug.

Quote:

We used shorts in most of the programs in class. Shorts occupy only 2 bytes in memory, instead of the 4, integers take. And the numbers used are so small, that they fit more than comfortably in a short var.
What is 2 bytes vs 4 bytes in today's memory? Besides, 2 bytes mean you have a lower margin for overflow, possibly leading to bugs (though, I admit, that is a weak argument).
But, point is, unless you are really tight on memory, don't bother with such optimizations.

Quote:

i wasn't aware I could make functions inside of structures. I'll look into it.
Well, to be honest, those are usually known as classes.

Quote:

I'm not familiar with the operator "::"
It's called the namespace operator. It's used to access types inside namespaces, as well as structs and classes.

Quote:

Are you making a structure a member of another structure. ?
That is some clever use of structures. I've never seen that before.

You are creating a structure inside of another structure, again I've never seen that before.That's a very clever way to do it :)
It's good practice, since the struct itself (or later, classes, as you shall learn) knows better than the outside world what data it stores, and therefore, it can define some types that makes sense to use to manipulate its data. It increases abstraction (and thus, code quality).

Quote:

I'm not too happy with that duplication of code either. I'm gonna see how can I make it into functions.

Are you suggesting to use an array to store the message strings, and then comparing the indexes in a loop.?
That would save me the time when checking, but I would still have to assign the string to each array index manually. Unless I use numbers instead of strings, as arguments for message, in which case the program would lose readability.
message("gameOver") is more meaningful that message(7).

Would you please explain why, and show me an alternative way to do it.
Code:

```switch (day) {         case MONDAY: return "Monday";         case TUESDAY: return "Tuesday";         // etc }```
You can simply do:
Code:

```const std::string Days[] = { "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday" }; return Days[day];```
Also, the idea was that instead of
message("gameOver")
you would do
message(GAME_OVER);
(Possibly message(Msgs::GameOver) if you have C++11 support; see strongly typed enums.)
Where GAME_OVER is a constant from a Messages enum.
• 09-08-2012
grumpy
Quote:

Originally Posted by Elysia
It's called the namespace operator. It's used to access types inside namespaces, as well as structs and classes.

I've never heard it called that before. It is usually referred to as the "scope resolution" operator, and is used for resolving names within named scopes (a scope is sometimes called a context in natural english). A namespace is one type of named scope. The name of a struct or class is another type (where the name is that of the struct/class type).
• 09-08-2012
whiteflags
Quote:

OK, so first, you can just type '┴', not 193. Makes it easier to read, don't you think?
Second, the problem with extended characters. They are not standard. ... So what do you do about it? You use Unicode. If you can't use Unicode, then complain to your Professor that you shouldn't use it. If that doesn't work, then drop the course.
This can't be worth more than 3 points on the rubric. By all means drop the course because you'll get an A- ...
Show 80 post(s) from this thread on one page
Page 1 of 3 123 Last