feedback on code design

Discussion in 'C++' started by krastavicakrastavica@abv.bg, May 30, 2012.

  1. Guest

    First time posting here, sorry if i failed :(
    So i wanted to write a Hangman game and i started it, as im newbie
    programmer it took me several days to track all the bugs etc.
    Meanwhile i want to get good habits on designing and i read that
    Getters and Setters arent the best way to go for a class design. I
    tryed my best to lead this project to a good design but i need a
    feedback to see if im on the right track, please, comment on
    everything you see that are lame or that can be build better.
    Im scared of comments like "looks good, decent design" etc. please be
    strict i really worked hard on it :D

    ..cpp
    Code:
    #include "HANGMAN.h"
    
    //Ctor and Dtor
    HANGMAN::HANGMAN(){
    
    
    SetConsoleTitle ("Hangman v1.0");
    hStdout = GetStdHandle (STD_OUTPUT_HANDLE);
    
    
    exitGame = false;
    inGame = false;
    
    
    lenofFile = CountLinesOfFile ("words.txt"); //Count lines
    CreateWordsBuf (lenofFile, "words.txt");    //Allocate and store
    them
    
    
    srand (time(NULL)); //seed for picking element index from
    _wordsBuf
    
    
    CreditsIntro();
    
    
    }
    HANGMAN::~HANGMAN(){
    //deallocate
    delete[] _wordsBuf;
    _wordsBuf = NULL;
    
    
    delete[] _hiddenWord;
    _hiddenWord = NULL;
    
    
    delete[] _letterBuf;
    _letterBuf = NULL;
    }
    
    
    
    
    void HANGMAN::Set_exitGame (bool b){ //Flag for the main loop
    exitGame = b;
    }
    bool HANGMAN::Get_exitGame(){
    return exitGame;
    }
    void HANGMAN::Set_inGame(bool b){    //Flag for inside game loop
    inGame = b;
    }
    bool HANGMAN::Get_inGame (){
    return inGame;
    }
    
    
    
    
    void HANGMAN::ClearScreen(){  //Clears the screen in radius {80,30}
    COORD homeCoords = {0,0};
    
    
    //{80,30} is enough to clear the previous game
    SetConsoleCursorPosition (hStdout, homeCoords);
    for (int row = 0; row < 30; row++){
    for (int col = 0; col < 80; col++){
    cout << ' ';
    }
    }
    SetConsoleCursorPosition (hStdout, homeCoords);
    }
    void HANGMAN::CreditsIntro(){ //Badass intro and tips
    COORD dotsCoords = {0,2};
    int tip = rand() % 2;
    
    
    cout << "GAME BY: COFFECAT" << endl << endl;
    
    
    //pick a tip
    cout << endl << endl << "   Tip of the day:" << endl;
    switch (tip){
    case 0:
    cout << "Separate the word's in the text file with a
    whitespace.";
    break;
    case 1:
    cout << "Don't let your user see the word's from the text
    file.";
    break;
    }
    
    
    //output 3 dots with short delay, clear and output again
    SetConsoleCursorPosition (hStdout, dotsCoords);
    for (int i = 0; i < 2; i++){
    cout << "."; Sleep (500);
    cout << "."; Sleep (500);
    cout << "."; Sleep (500);
    cout << '\r' << "   ";
    cout << '\r';  Sleep(500);
    }
    
    
    ClearScreen();
    }
    
    
    
    
    int HANGMAN::CountLinesOfFile (const char* fname){ //Counts number of
    words in .txt file
    
    
    int n = 0;
    string word;
    
    
    ifstream file (fname);
    
    
    //read all lines form the file and count them
    if (file.is_open()){
    
    
    while (!file.eof()){
    getline (file, word, ' ');
    n++;
    }
    file.close();
    }
    else{
    cout << "Failed to open file for CountLinesOfFile (const
    char*)" << endl;
    }
    
    
    return n;
    }
    void HANGMAN::CreateWordsBuf (int len, const char* fname){ //Allocate
    _wordsBuf and store words .txt
    string line;
    int index = 0;
    _wordsBuf = new string[len];
    
    
    ifstream file (fname);
    
    
    //read words and store them in own element
    if (file.is_open()){
    
    
    while (!file.eof()){
    
    
    getline (file, line, ' ');
    _wordsBuf[index++] = line;
    }
    file.close();
    }
    else{
    cout << "Failed to open words.txt for CreateWordBuf (int,
    const char*)" << endl;
    }
    }
    
    
    
    
    void HANGMAN::Pull_randWord (int filelen){ //Pulls a word from
    _wordsBuf
    int randVal;
    randVal = rand() % filelen;
    
    
    randWord = _wordsBuf[randVal];
    }
    void HANGMAN::CreateLetterBuf(int len){    //Allocate _letterBuf and
    fills with whitespaces
    
    
    //allocate array and fill it whitespaces (inputed letters will be
    pushed)
    _letterBuf = new char[len];
    for (int i = 0; i < len; i++){
    _letterBuf[i] = ' ';
    }
    }
    void HANGMAN::CreateHiddenWord(int len){   //Allocate _hiddenWord buf
    and fills with underscores
    
    
    //allocate array and fill it with underscores for hidden word UI
    _hiddenWord = new char[len];
    for (int i = 0; i < len; i++){
    _hiddenWord[i] = '_';
    }
    }
    void HANGMAN::OutputHiddenWord(){ //Outputs _hiddenWord buf
    
    
    //outputs the underscores and adds a whitespace between each for
    better reading
    for (int x = 0; x < lenofRW; x++){
    cout << _hiddenWord[x];
    if (x != lenofRW - 1){
    cout << " ";
    }
    }
    }
    void HANGMAN::OutputIngameUI(){   //Requires _hiddenWord buf to be
    allocated and output simple UI
    
    
    cout << "Tries left: " << triesLeft;
    cout << endl << endl;
    
    
    OutputHiddenWord();
    cout << endl << endl;
    
    
    cout << "Letter input: ";
    }
    void HANGMAN::UpdateHiddenWord(){ //Set cursor at proper pos and re-
    writes _hiddenWord buf
    COORD hiddenWordCoords = {0,2};
    
    
    SetConsoleCursorPosition (hStdout, hiddenWordCoords);
    OutputHiddenWord();
    }
    void HANGMAN::UpdateTriesLeft(){  //Set cursor at proper pos and re-
    write triesLeft value
    COORD triesLeftCoords = {0,0};
    COORD eraseDigit = {13,0};
    
    
    //put whitespace at the second digit when triesLeft goes under 9
    if (triesLeft < 10){
    SetConsoleCursorPosition (hStdout, eraseDigit);
    cout << ' ';
    }
    SetConsoleCursorPosition (hStdout, triesLeftCoords);
    cout << "Tries left: " << triesLeft;
    cout << endl << endl;
    }
    
    
    
    
    void HANGMAN::CreateNewGame(){   //Pulls new randWord, alloc bufs,
    value for triesLeft/lenofLetterB
    
    
    ClearScreen();
    
    
    //pull new randWord and create _hiddenWord buf
    Pull_randWord (lenofFile);
    lenofRW = randWord.size();
    CreateHiddenWord (lenofRW);
    
    
    //set value for triesLeft and create _letterBuf buf
    triesLeft = lenofRW;
    lenofLetterB = triesLeft * 2; //hold correct and incorect letters
    CreateLetterBuf (lenofLetterB);
    
    
    inGame = true;
    }
    void HANGMAN::CheckForEndGame(){ //Compares randWord letters with
    _hidenWord buf, checks triesLeft
    
    
    //check if triesLeft are wasted, if so, end the current game
    if (triesLeft == 0){
    cout << endl << endl << endl << endl << "WRONG!";
    Sleep (2000);
    inGame = false;
    }
    
    
    //compare _hiddenWord and randWord if all letters match
    int count = 0;
    for (int j = 0; j < lenofRW; j++){
    if (_hiddenWord[j] == randWord[j]){
    count++;
    }
    }
    //if _hiddenWord match randWord, end the current game
    if (count == lenofRW){
    cout << endl << endl << endl << endl << "CORRECT!";
    Sleep (2000);
    inGame = false;
    }
    }
    
    
    
    
    void HANGMAN::Set_input(char val){ //Used for reset
    input = val;
    }
    char HANGMAN::Get_input(){
    return input;
    }
    void HANGMAN::AskInputGetch (){ //Works with getch()
    input = getch();
    }
    void HANGMAN::AskLetterInput(){ //Ask for single letter, can delete it
    before confirming with Enter
    COORD letterInpCoords; //X and Y values will be changed trough the
    function loops
    COORD letterInpOrigCoords = {14,4}; //permanent values for X and Y
    bool letterConfirmed = false;
    
    
    //SetConsoleCursorPosition (hStdout, letterInpOrigCoords); //set
    cursor at begining
    
    
    //loop until a letter is pressed and then ENTER key
    while (!letterConfirmed){
    
    
    //if letter is deleted, reset coords so can re-write the
    buffer
    letterInpCoords.X = 14;
    letterInpCoords.Y = 4;
    
    
    //search free element spot for input
    for (int i = 0; i < lenofLetterB; i++){
    if (_letterBuf[i] == ' '){  //if finds a whitespace
    element,
    letterInpCoords.X += i; //take value of i + 14 for X
    coordinates
    SetConsoleCursorPosition (hStdout, letterInpCoords); //
    set proper pos for input
    
    
    //loop until a letter only is pressed
    do {
    inpLetter = getch();
    } while ((inpLetter != 'A' && inpLetter != 'a') &&
    (inpLetter != 'B' && inpLetter != 'b') &&
    (inpLetter != 'C' && inpLetter != 'c') &&
    (inpLetter != 'D' && inpLetter != 'd') &&
    (inpLetter != 'E' && inpLetter != 'e') &&
    (inpLetter != 'F' && inpLetter != 'f') &&
    (inpLetter != 'G' && inpLetter != 'g') &&
    (inpLetter != 'H' && inpLetter != 'h') &&
    (inpLetter != 'I' && inpLetter != 'i') &&
    (inpLetter != 'J' && inpLetter != 'j') &&
    (inpLetter != 'K' && inpLetter != 'k') &&
    (inpLetter != 'L' && inpLetter != 'l') &&
    (inpLetter != 'M' && inpLetter != 'm') &&
    (inpLetter != 'N' && inpLetter != 'n') &&
    (inpLetter != 'O' && inpLetter != 'o') &&
    (inpLetter != 'P' && inpLetter != 'p') &&
    (inpLetter != 'Q' && inpLetter != 'q') &&
    (inpLetter != 'R' && inpLetter != 'r') &&
    (inpLetter != 'S' && inpLetter != 's') &&
    (inpLetter != 'T' && inpLetter != 't') &&
    (inpLetter != 'U' && inpLetter != 'u') &&
    (inpLetter != 'V' && inpLetter != 'v') &&
    (inpLetter != 'W' && inpLetter != 'w') &&
    (inpLetter != 'X' && inpLetter != 'x') &&
    (inpLetter != 'Y' && inpLetter != 'y') &&
    (inpLetter != 'Z' && inpLetter != 'z'));
    
    
    break; //break loop if letter is inputed
    }
    //if any time comma's are used or other letters, count
    them
    else
    letterInpCoords.X += 1;
    }
    
    
    
    
    //output inputed letter on the same coords that was asked to
    cout << inpLetter;
    
    
    //loop until ENTER or BACKSPACE key is pressed
    input = -1; //reset
    while (input != 13 && input != 8){
    
    
    input = getch();
    switch (input){
    //ENTER KEY -- confirms the letter
    case 13:
    /* save inputed letter in first whitespace element
    and add coma after it.
    Confirm the letter flag to true and exit the
    function*/
    for (int i = 0; i < lenofLetterB; i++){
    if (_letterBuf[i] == ' '){
    _letterBuf[i] = inpLetter;
    cout << ",";
    letterConfirmed = true;
    break;
    }
    }
    break; //case ENTER: ends
    
    
    
    
    //BACKSPACE KEY -- re-printing letters/visualy
    deleting
    case 8:
    /* if still no letters in buffer, set cursor to
    begining and output whitespace,
    then back one char so input can be be on the same
    spot */
    if (_letterBuf[0] == ' '){
    SetConsoleCursorPosition (hStdout,
    letterInpOrigCoords);
    cout << ' ' << '\b';
    }
    /* if have inputed letter, set cursor to begining
    and re-write all letter's
    and comma's */
    else if (_letterBuf[0] != ' '){
    SetConsoleCursorPosition (hStdout,
    letterInpOrigCoords);
    for (int i = 0; i < lenofLetterB; i++){
    if (_letterBuf[i] != ' '){        //if
    have letter in the element
    cout << _letterBuf[i] << ","; //output
    it including comma after it
    }
    else {            //if no letter in the
    element
    cout << ' ' << '\b'; //output
    whitespace to simulate deletion
    break;        //then back one char so
    input be on the same spot
    }
    }
    }
    
    
    break; //case BACKSPACE: ends
    
    
    } //switch (input) ends
    
    
    } //while (input != ENTER && != BACKSPACE) ends
    
    
    } //while (!letterConfirmed) ends
    
    
    }
    void HANGMAN::CheckInputLetter(){ //Check if letter exist in randWord
    bool matchLetter = false;
    
    
    //compares inpLetter with randWord letters
    for (int x = 0; x < lenofLetterB; x++){
    if (inpLetter == randWord[x]){
    _hiddenWord[x] = inpLetter;
    matchLetter = true;
    }
    }
    //if inpLetter dont exist in randWord, decrease triesLeft
    if (!matchLetter){
    --triesLeft;
    }
    }
    ..h
    Code:
    #ifndef HANGMAN_H#define HANGMAN_H
    #include <iostream>
    #include <fstream>
    #include <conio.h>
    #include <windows.h>
    #include <string>
    #include <time.h>
    
    
    using namespace std;
    
    
    class HANGMAN
    {
    public:
    HANGMAN(); //Ctor and Dtor
    virtual ~HANGMAN();
    
    
    void Set_exitGame (bool b); //Flag for the main loop
    bool Get_exitGame();
    void Set_inGame (bool b);   //Flag for inside game loop
    bool Get_inGame();
    
    
    void ClearScreen();  //Clears the screen in radius {80,30}
    void CreditsIntro(); //Badass intro and tips
    
    
    int CountLinesOfFile (const char* fname); //Count lines
    of .txt file
    void CreateWordsBuf (int len, const char* fname);  //Allocates
    _wordsBuf and store words.txt
    
    
    void Pull_randWord (int filelen); //Pulls a word from
    _wordsBuf
    void CreateLetterBuf(int len);    //Allocate _letterBuf and
    fills with whitespaces
    void CreateHiddenWord (int len);  //Allocate _hiddenWord buf
    and fills with underscores
    void OutputHiddenWord(); //Outputs _hiddenWord buf
    void OutputIngameUI();   //Requires _hiddenWord buf to be
    allocated and output simple UI
    void UpdateHiddenWord(); //Set cursor at proper pos and re-
    writes _hiddenWord buf
    void UpdateTriesLeft();  //Set cursor at proper pos and re-
    write triesLeft value
    
    
    void CreateNewGame();  //Pulls new randWord, alloc bufs, value
    for triesLeft/lenofLetterB
    void CheckForEndGame(); //Compares randWord letters with
    _hidenWord buf, checks triesLeft
    
    
    void Set_input (char v); //Used for reset
    char Get_input();
    void AskInputGetch();   //Works with getch()
    void AskLetterInput();  //Ask for single letter, can delete it
    before confirming with Enter
    void CheckInputLetter(); //Check if letter exist in randWord
    
    
    
    
    private:
    //windows predef data types
    HANDLE hStdout; //std output handle
    
    
    //game
    int triesLeft;
    bool exitGame; //flag for the main loop
    bool inGame;   //flag for inside game loop
    
    
    //input
    char input;     //UI input
    char inpLetter; //input only for letters
    char* _letterBuf;
    int lenofLetterB; //lenth of _letterBuf
    
    
    //file
    int lenofFile; //lenth of .txt file
    string* _wordsBuf;
    
    
    //word
    char* _hiddenWord;
    string randWord; //Pull_randWord returns this variable
    int lenofRW;     //lenth of randWord
    };
    
    
    #endif // HANGMAN_H
    ..cpp
    Code:
    #include "HANGMAN.h"
    
    using namespace std;
    
    
    int main()
    {
    HANGMAN HANGM;
    
    
    while (!HANGM.Get_exitGame()){
    
    
    HANGM.ClearScreen();
    cout << "HANGMAN MENU:" << endl;
    cout << "------------" << endl;
    cout << "New Game -- Enter Key" << endl;
    cout << "Exit Game -- Esc Key" << endl << endl;
    
    
    //Loop until in MENU
    HANGM.Set_input (-1); //reset input
    while (HANGM.Get_input() != 13 && HANGM.Get_input() != 27){
    
    
    //Ask for user input
    HANGM.AskInputGetch();
    switch (HANGM.Get_input()){
    
    
    //New Game -- Enter Key
    case 13:
    HANGM.CreateNewGame();
    HANGM.OutputIngameUI();
    break;
    
    
    //Exit Game -- Esc Key
    case 27:
    HANGM.Set_exitGame (true);
    break;
    
    
    default:
    break;
    
    
    } //switch (input) ends
    
    
    } //while (ENTER && ESC) ends
    
    
    //Start Game loop
    while (HANGM.Get_inGame()){
    
    
    HANGM.AskLetterInput();
    HANGM.CheckInputLetter();
    
    
    HANGM.UpdateTriesLeft();
    HANGM.UpdateHiddenWord();
    
    
    HANGM.CheckForEndGame();
    
    
    } //while (inGame) ends
    
    
    } //while (!exitGame) ends
    
    
    
    
    return 0;
    }
    Feedback on design, comments, names, everything that you see that can
    be done in a better understanding way. How would you work with the
    function prototypes if you didnt had access to .cpp file? etc, please
    on everything
    , May 30, 2012
    #1
    1. Advertising

  2. Guest

    On Wednesday, May 30, 2012 10:52:11 AM UTC+1, wrote:

    > [...] i wanted to write a Hangman game [...] i want to get good
    > habits on designing [...]


    > i read that
    > Getters and Setters arent the best way to go for a class design.


    this is slightly tricky. Or rather, a rule like "don't use getters and setters" is more often wrong than right. OO Design (class design) is about designing the public interface ("what does it do?", "how does it appear to its users (client)?") and only then designing the internals (implementaion). implementaion details should be hidden from the client. If clients need to see a particular variable the write a getter(), if they need to update then asetter() (you could always put some validation code in as well). If it's only an implementation detail then don't expose it to the outside world.

    > I
    > tryed my best to lead this project to a good design but i need a
    > feedback to see if im on the right track, please, comment on
    > everything you see that are lame or that can be build better.
    > Im scared of comments like "looks good, decent design" etc. please be
    > strict i really worked hard on it :D


    I usually like to look at the class declaration first.
    These are the member variables

    private:
    //windows predef data types
    > HANDLE hStdout; //std output handle


    good design would separate Windows stuff from application stuff.


    > //game
    > int triesLeft;
    > bool exitGame; //flag for the main loop
    > bool inGame; //flag for inside game loop
    >
    > //input
    > char input; //UI input
    > char inpLetter; //input only for letters
    > char* _letterBuf;


    a mix of char* and std::string is odd...

    > int lenofLetterB; //lenth of _letterBuf
    >
    > //file
    > int lenofFile; //lenth of .txt file
    > string* _wordsBuf;


    consider

    vector<string> wordsBuf;

    >
    >
    > //word
    > char* _hiddenWord;


    why a char* ?

    > string randWord; //Pull_randWord returns this variable
    > int lenofRW; //lenth of randWord


    just use
    randWord.length()

    > .cpp
    >
    Code:
    #include "HANGMAN.h"
    > 
    > //Ctor and Dtor
    > HANGMAN::HANGMAN(){[/color]
    
    it's unusual to use all uppercase for a class name. I'd use Hangman::.
    I also don't like comments like "Ctor"; I *kmow* it's a contructor 
    I'm a C++ programmer.
    
    Its better to write your constructors with an initialisation list. 
    I'd also initialise *all* member variables (though not everyone agrees)
    
    HANGMAN::HANGMAN():
    hStdout(GetStdHandle (STD_OUTPUT_HANDLE)),
    triesLeft(),       // you tell me!
    exitGame(false),
    inGame(false),
    input(),
    inpLetter(),
    _letterBuf(),
    lenofLetterB(),
    lenofFile(CountLinesOfFile ("words.txt")),
    _wordsBuf(),
    _hiddenWord(),
    randWord(),
    lenofRW()
    {
    SetConsoleTitle ("Hangman v1.0"); 
    CreateWordsBuf (lenofFile, "words.txt");
    srand (time(NULL));
    CreditsIntro();
    }
    
    your CTOR looks like it's doing rather a lot.
    
    [color=blue]
    >     SetConsoleTitle ("Hangman v1.0");
    >     hStdout = GetStdHandle (STD_OUTPUT_HANDLE);
    > 
    >     exitGame = false;
    >     inGame = false;
    > 
    > 
    >     lenofFile = CountLinesOfFile ("words.txt"); //Count lines
    >     CreateWordsBuf (lenofFile, "words.txt");    //Allocate and store
    > them
    > 
    > 
    >     srand (time(NULL)); //seed for picking element index from
    > _wordsBuf
    > 
    > 
    >     CreditsIntro();
    > 
    > 
    > }
    > HANGMAN::~HANGMAN(){
    >     //deallocate
    >     delete[] _wordsBuf;
    >     _wordsBuf = NULL;[/color]
    
    why set to NULL?
    
    [color=blue]
    >     delete[] _hiddenWord;
    >     _hiddenWord = NULL;
    > 
    > 
    >     delete[] _letterBuf;
    >     _letterBuf = NULL;
    > }[/color]
    
    if you used vector rather than pointers you'd have less work to do in the DTOR.
    Eventually you'll find out about Exception Safety and new/delete simply aren't safe.
    
    [color=blue]
    > void HANGMAN::Set_exitGame (bool b){ //Flag for the main loop
    >     exitGame = b;
    > }
    > bool HANGMAN::Get_exitGame(){
    >     return exitGame;
    > }
    > void HANGMAN::Set_inGame(bool b){    //Flag for inside game loop
    >     inGame = b;
    > }
    > bool HANGMAN::Get_inGame (){
    >     return inGame;
    > }[/color]
    
    getters and setters. These same fairly sensible.
    
    Got bored round about here
    
    <snip>
    
    until...
    [color=blue]
    >                 //loop until a letter only is pressed
    >                 do {
    >                     inpLetter = getch();
    >                 } while ((inpLetter != 'A' && inpLetter != 'a') &&
    >                          (inpLetter != 'B' && inpLetter != 'b') &&
    >                          (inpLetter != 'C' && inpLetter != 'c') &&[/color]
    
    ....eek!
    
    If you ever find yourself typeing the same code over and 
    over again ask yourself "is there a better way?".
    
    Well you could test if the character is in a list.
    while (strchr ("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz",inpLetter) != NULL)
     
    or
    while (isalpha (inpLetter))
     
    these are both in the standard library. I'd use isalpha() and gave the first 
    solution to suggest there is almost always a better way.
    [color=blue]
    >                 break; //break loop if letter is inputed
    >             }
    >             //if any time comma's are used or other letters, count
    > them
    >             else
    >                 letterInpCoords.X += 1;
    >         }
    > 
    > 
    > 
    > 
    >         //output inputed letter on the same coords that was asked to
    >         cout << inpLetter;
    > 
    > 
    >         //loop until ENTER or BACKSPACE key is pressed
    >         input = -1; //reset
    >         while (input != 13 && input != 8){[/color]
    
    don't embed constants in you code use '\n' and '\b'
    [color=blue]
    >             input = getch();
    >             switch (input){
    >                 //ENTER KEY -- confirms the letter
    >                 case 13:
    >                     /* save inputed letter in first whitespace element[/color]
    
    <snip>
    
    [color=blue]
    > Feedback on design, comments, names, everything that you see that can
    > be done in a better understanding way. How would you work with the
    > function prototypes if you didnt had access to .cpp file? [/color]
    
    what? Put them in the header file?
    
    etc, please[color=blue]
    > on everything[/color]
    
    use std::vector avoid char* try to do one thing (and only one thing) 
    well in each function or class.
    , May 30, 2012
    #2
    1. Advertising

  3. Guest

    On 30 Май, 15:30, wrote:
    > good design would separate Windows stuff from application stuff.

    where to put them?

    > >         //word
    > >         char* _hiddenWord;

    >
    > why a char* ?

    its a char buffer that i will store inpLetter chars if they are
    correct, then re-write that buffer and show wich was correct and wich
    not. Saving letters in that buffer

    > it's unusual to use all uppercase for a class name. I'd use Hangman::.

    I though functions to start with uppercase followed by small ones, and
    all classes with uppercase, its like i say that class is the leader
    followed by function, then a pointer (*_ptr //with underscore),
    followed by smallcase normal variable. Something like ranking :?

    > Its better to write your constructors with an initialisation list.
    > I'd also initialise *all* member variables (though not everyone agrees)

    What do you mean, please explain some more here

    > HANGMAN::HANGMAN():
    >     hStdout(GetStdHandle (STD_OUTPUT_HANDLE)),
    >     triesLeft(),       // you tell me!
    >     exitGame(false),
    >     inGame(false),
    >     input(),
    >     inpLetter(),
    >     _letterBuf(),
    >     lenofLetterB(),
    >     lenofFile(CountLinesOfFile ("words.txt")),
    >     _wordsBuf(),
    >     _hiddenWord(),
    >     randWord(),
    >     lenofRW()
    > {
    >     SetConsoleTitle ("Hangman v1.0");
    >     CreateWordsBuf (lenofFile, "words.txt");
    >     srand (time(NULL));
    >     CreditsIntro();
    >
    > }
    >
    > your CTOR looks like it's doing rather a lot.

    can you also explain a bit more on this one, sir.

    > if you used vector rather than pointers you'd have less work to do in theDTOR.
    > Eventually you'll find out about Exception Safety and new/delete simply aren't safe.

    Thanks for this one, will read some more about those operators, maybe
    you have a good link in hand?

    >
    > If you ever find yourself typeing the same code over and
    > over again ask yourself "is there a better way?".
    >
    > Well you could test if the character is in a list.
    >      while (strchr ("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz", inpLetter) != NULL)
    >
    > while (isalpha (inpLetter))
    >
    > these are both in the standard library. I'd use isalpha() and gave the first
    > solution to suggest there is almost always a better way.

    nice one. thanks

    > don't embed constants in you code use '\n' and '\b'

    can you explain me some more on this one please.
    ER KEY -- confirms the letter

    > what? Put them in the header file?

    i mean, can you code a hangman game only seeing the prototype
    functions? With no acces to the .cpp file

    > use std::vector avoid char* try to do one thing (and only one thing)
    > well in each function or class.


    Thanks alot sir, great feedback, learned alot, dont stop if possible
    , May 30, 2012
    #3
  4. I apologize for not having the patience of going through the entirety of
    your code, but I will comment on your constructor/destructor design.

    wrote:
    > .cpp
    >
    Code:
    #include "HANGMAN.h"
    >
    > //Ctor and Dtor
    > HANGMAN::HANGMAN(){[/color]
    
    Since your question is about code design (not just correctness), I would
    point out that, while it's ultimately a question of personal preferences
    and coding conventions, it's generally customary to reserve all-caps names
    for precompiler macros, and class names are usually in CamelCase (or, at
    the very least, not in all-caps).
    
    Of course the compiler itself doesn't care either way (unless there's a
    name clash), but it can help other people reading your code.
    
    Also, with regards to code correctness, you are not initializing all of
    the member variables in your constructor. This is inadvisable because the
    compiler is not forced to implicitly initiaize basic types (such as pointers)
    and in fact most compilers won't. (This means that pointers will generally
    *not* be initialized to null if you don't do it explicitly, which means they
    will *not* be safe to delete, if so left uninitialized.)
    
    If you have an instance of a class (such as std::string) as a member variable,
    that doesn't need to be explicitly initialized in your constructor because
    it has a constructor of its own, and that is guaranteed to be called
    implicitly. (The only thing that's not guaranteed to be implicitly
    initialized is basic types such as ints and pointers.)
    [color=blue]
    >    SetConsoleTitle ("Hangman v1.0");
    >    hStdout = GetStdHandle (STD_OUTPUT_HANDLE);[/color]
    
    This is not standard C++ (unless those functions really are part of your
    code, but it looks more like Windows-specific stuff) so it's really out of
    the scope of this newsgroup. However, calling that code in the constructor
    of a class is dubious design. That kind of code should be called at the
    start of your program execution, and only once. The most usual place to
    put that code in is in the main() function of your program.
    [color=blue]
    >    exitGame = false;
    >    inGame = false;[/color]
    
    While initializing such member functions in the body of your constructor
    is not wrong per se, it's usually recommended to do it with an initializer
    list instead. (This makes sure that they are initialized before they are
    used, and it gives better opportunities for the compiler to optimize your
    code.)
    [color=blue]
    >    lenofFile = CountLinesOfFile ("words.txt"); //Count lines
    >    CreateWordsBuf (lenofFile, "words.txt");    //Allocate and store
    > them[/color]
    
    You are allocating dynamic memory at the end of a pointer in your class,
    but you have neither disabled the copy constructor and copy assignment
    operators, nor implemented them. This is prone to errors. If you
    explicitly allocate dynamic memory like this, you should definitely do
    either one of those things (ie. either disable the copy constructor and
    copy assignment operators, or implement them appropriately). Leaving them
    to compiler-defaults breaks your class (except in some extremely specific
    situations, but this is not such a situation).
    
    The most advisable solution to this is that if you don't really need to
    explicitly allocate dynamic memory, you use one of the standard data
    containers instead, usually std::string or std::vector for sequential
    data. Since those classes have all of their constructors and assignment
    operators well defined, your code automatically becomes correct without
    you having to implement those for your class.
    
    However, if you really must allocate the memory explicitly yourself, then
    at the very least disable the copy constructor and copy assignment operator
    of your class. This makes sure that it cannot be accidentally broken (because
    instances of your class cannot then be copied nor assigned).
    
    If your class must be copyable, and you absolutely must allocate the memory
    explicitly yourself, then it becomes a bit complicated because then you must
    implement the copy constructor and copy assignment operators for your class
    to work properly, and this is not a trivial thing to do, especially for a
    total beginner. If you can, avoid this for now and learn it later. (Your
    code leads me to believe that this class doesn't actually need to be copied,
    so the easiest way is to simply disable them.)
    
    (Yes, this can be a bit of a drag with C++ sometimes because it's not a
    garbage-collected language like most other OO languages, but it's not that
    big of a deal when you get used to it.)
    [color=blue]
    >    srand (time(NULL)); //seed for picking element index from
    > _wordsBuf
    >
    >
    >    CreditsIntro();[/color]
    
    Again something that is dubious to do in a constructor, and looks more like
    code that should be in main().
    Juha Nieminen, May 31, 2012
    #4
  5. Guest

    On Wednesday, May 30, 2012 9:25:15 PM UTC+1, wrote:
    > On 30 Май, 15:30, wrote:


    Have you read juha's response? some of what we say overlaps

    > > good design would separate Windows stuff from application stuff.

    >
    > where to put them?


    perhaps another class?

    > > >         //word
    > > >         char* _hiddenWord;

    > >
    > > why a char* ?

    >
    > its a char buffer that i will store inpLetter chars if they are
    > correct, then re-write that buffer and show wich was correct and wich
    > not. Saving letters in that buffer


    consider std::string or std::vector<char>

    > > it's unusual to use all uppercase for a class name. I'd use Hangman::.

    >
    > I [thought?] functions to start with uppercase followed by small ones, and
    > all classes with uppercase, its like i say that class is the leader
    > followed by function, then a pointer (*_ptr //with underscore),
    > followed by smallcase normal variable.


    technically you shouldn't use identifiers that start with _ as they may clash with identifiers reserved for the implementation.

    there are many conventions for C++ but yours is a bit unusual. Stroustrup for instance uses

    Class, function(), variable

    He seems to use long_identifier though some use longIdentifier

    > Something like ranking :?


    what?

    > > Its better to write your constructors with an initialisation list.
    > > I'd also initialise *all* member variables (though not everyone agrees)

    >
    > What do you mean, please explain some more here


    um. read this:-

    > > HANGMAN::HANGMAN():
    > >     hStdout(GetStdHandle (STD_OUTPUT_HANDLE)),
    > >     triesLeft(),       // you tell me!
    > >     exitGame(false),
    > >     inGame(false),
    > >     input(),
    > >     inpLetter(),
    > >     _letterBuf(),
    > >     lenofLetterB(),
    > >     lenofFile(CountLinesOfFile ("words.txt")),
    > >     _wordsBuf(),
    > >     _hiddenWord(),
    > >     randWord(),
    > >     lenofRW()
    > > {
    > >     SetConsoleTitle ("Hangman v1.0");
    > >     CreateWordsBuf (lenofFile, "words.txt");
    > >     srand (time(NULL));
    > >     CreditsIntro();
    > > }


    many of those identifiers I didn't know what to initialise them to. But they should be initialise to *something*. Build in classes like string and vector have default ctors that set them to sane values.

    man-millenia have been expended on finding "uninitialised variable" bugs

    > > your CTOR looks like it's doing rather a lot.

    >
    > can you also explain a bit more on this one, sir.


    Juha had a suggestion.

    > > if you used vector rather than pointers you'd have less work to
    > > do in the DTOR. Eventually you'll find out about Exception Safety
    > > and new/delete simply aren't safe.

    >
    > Thanks for this one, will read some more about those operators, maybe
    > you have a good link in hand?


    basically memory management should be handled by specialised classes. eg. std::autoptr or one of the boost "smart" pointers

    I found this to be handy on exception safety
    http://www.octopull.co.uk/c /dragons/

    <snip>

    > > How would you work with the
    > > function prototypes if you didnt had access to .cpp file?

    >
    > > what? Put them in the header file?

    >
    > i mean, can you code a hangman game only seeing the prototype
    > functions? With no acces to the .cpp file


    yes. Include the .h file in your application and you can define instances of the class and call its public member functions. This is the Right Way to do things.

    > > use std::vector avoid char* try to do one thing (and only one thing)
    > > well in each function or class.

    >
    > Thanks alot sir, great feedback, learned alot, dont stop if possible


    Have you access to a good text book? I've always liked Stroustrup, "Accelerated C++" is well thought of.
    , May 31, 2012
    #5
  6. <> wrote:
    > I though functions to start with uppercase followed by small ones, and
    > all classes with uppercase, its like i say that class is the leader
    > followed by function, then a pointer (*_ptr //with underscore),
    > followed by smallcase normal variable. Something like ranking :?


    Those are at least unusual naming conventions, i would rather use:

    ALL_UPPERCASE for
    - macro names (#define)

    StartWithUppercase for
    - classes (not sure for other types like structs, unions, typedefs, ...)
    - functions/methods ("Microsoft style", e.g. WinAPI, .Net, MFC, ATL etc.)

    startWithLowercase for
    - functions/methods
    - variables/parameters

    _startWithUnderscoreAndLowercase is theoretically allowed in C++,
    personally I wouldn't use it anyways. Some use it for
    - private members (probably also Microsoft style)
    - static functions

    _StartWithUnderscoreAndUppercase and
    __startWithDoubleUnderscore are reserved for
    - implementation of compilers and standard libraries

    Tobi
    Tobias Müller, May 31, 2012
    #6
  7. Guest

    On Thursday, May 31, 2012 5:19:09 PM UTC+1, Tobias Müller wrote:
    > <> wrote:
    > > I though functions to start with uppercase followed by small ones, and
    > > all classes with uppercase, its like i say that class is the leader
    > > followed by function, then a pointer (*_ptr //with underscore),
    > > followed by smallcase normal variable. Something like ranking :?

    >
    > Those are at least unusual naming conventions, i would rather use:
    >
    > ALL_UPPERCASE for
    > - macro names (#define)
    >
    > StartWithUppercase for
    > - classes (not sure for other types like structs, unions, typedefs, ...)
    > - functions/methods ("Microsoft style", e.g. WinAPI, .Net, MFC, ATL etc.)
    >
    > startWithLowercase for
    > - functions/methods
    > - variables/parameters
    >
    > _startWithUnderscoreAndLowercase is theoretically allowed in C++,
    > personally I wouldn't use it anyways. Some use it for
    > - private members (probably also Microsoft style)
    > - static functions
    >
    > _StartWithUnderscoreAndUppercase and
    > __startWithDoubleUnderscore are reserved for
    > - implementation of compilers and standard libraries


    its also handy to label member variables in some way. I've seen

    _member, member_ and m_member
    , Jun 1, 2012
    #7
  8. Guest

    Thanks all, i received a good feedback and im willing to start re-
    designing this piece of code. There is things that i didnt know and i
    just read some of them.
    Ill start with the design of the constructor and the initialization
    list, here is one of my questions:
    i read its good all variables to be in the initialization list, but,
    for example, 'lenofFile' i have to use the function
    'CountLinesOfFile(...) to initialize 'lenofFile to it. Can i use
    functions inside it and is it good? (I didnt saw any initialization
    list do show this, but i read just few tutorials).
    , Jun 2, 2012
    #8
  9. Ian Collins Guest

    On 06/ 2/12 11:38 AM, wrote:
    > Thanks all, i received a good feedback and im willing to start re-
    > designing this piece of code. There is things that i didnt know and i
    > just read some of them.
    > Ill start with the design of the constructor and the initialization
    > list, here is one of my questions:
    > i read its good all variables to be in the initialization list, but,
    > for example, 'lenofFile' i have to use the function
    > 'CountLinesOfFile(...) to initialize 'lenofFile to it. Can i use
    > functions inside it and is it good? (I didnt saw any initialization
    > list do show this, but i read just few tutorials).


    Another guideline - avoid abbreviations - lengthOfFile is better.

    Yes, you can call functions, other than virtual ones, anywhere in a
    constructor.

    --
    Ian Collins
    Ian Collins, Jun 2, 2012
    #9
  10. wrote:
    > its also handy to label member variables in some way. I've seen
    >
    > _member, member_


    Personally I dislike those because they only add to the obfuscation of
    the code (especially when mixed with operators), and because the
    convention is single-purpose (ie. the same convention cannot be used
    to distinguish from class variables, local variables and variables
    inside namespaces, named or unnamed).
    Juha Nieminen, Jun 2, 2012
    #10
  11. Guest

    On Saturday, June 2, 2012 12:14:48 PM UTC+1, Juha Nieminen wrote:
    > wrote:


    > > its also handy to label member variables in some way. I've seen
    > >
    > > _member, member_

    >
    > Personally I dislike those because they only add to the obfuscation of
    > the code (especially when mixed with operators), and because the
    > convention is single-purpose (ie. the same convention cannot be used
    > to distinguish from class variables, local variables and variables
    > inside namespaces, named or unnamed).


    I tend to agree. I used to use member_ but I'm not so keen on it now. Other people find it obscure for instance.

    Some people use this->member but I'm not keen on that either. I think I've seen myMember but again not one I like.
    , Jun 2, 2012
    #11
  12. Ian Collins Guest

    On 06/ 3/12 02:18 AM, wrote:
    > On Saturday, June 2, 2012 12:14:48 PM UTC+1, Juha Nieminen wrote:
    >> wrote:

    >
    >>> its also handy to label member variables in some way. I've seen
    >>>
    >>> _member, member_

    >>
    >> Personally I dislike those because they only add to the obfuscation of
    >> the code (especially when mixed with operators), and because the
    >> convention is single-purpose (ie. the same convention cannot be used
    >> to distinguish from class variables, local variables and variables
    >> inside namespaces, named or unnamed).

    >
    > I tend to agree. I used to use member_ but I'm not so keen on it now. Other people find it obscure for instance.
    >
    > Some people use this->member but I'm not keen on that either. I think I've seen myMember but again not one I like.


    I've never bothered with member variable decoration. It should be clear
    from the context what is and what isn't a member. It's even easier
    these days with highlighting editors.

    --
    Ian Collins
    Ian Collins, Jun 2, 2012
    #12
  13. Ian Collins <> wrote:
    > I've never bothered with member variable decoration. It should be clear
    > from the context what is and what isn't a member.


    Maybe it should, but it that's quite an impossible demand to make in
    all cases.

    When you start mixing function-local variables, member variables, class
    variables, compilation-unit-local variables and compile-time constants in
    the same code, it quickly becomes confusing what is what, if you can't
    distinguish it from their name.

    The code may be very clear as you write it, and even the next day, but
    return to the code some months or even years later, and you'll often notice
    yourself repatedly thinking "where was this declared again? Was this a
    member variable or what?" When you find yourself doing that, it's a clear
    sign that you really need to start using a naming convention.

    > It's even easier these days with highlighting editors.


    The clarity of code should not depend on having an editor clarifying it
    for you.
    Juha Nieminen, Jun 3, 2012
    #13
  14. Ian Collins Guest

    On 06/ 3/12 07:12 PM, Juha Nieminen wrote:
    > Ian Collins<> wrote:
    >> I've never bothered with member variable decoration. It should be clear
    >> from the context what is and what isn't a member.

    >
    > Maybe it should, but it that's quite an impossible demand to make in
    > all cases.
    >
    > When you start mixing function-local variables, member variables, class
    > variables, compilation-unit-local variables and compile-time constants in
    > the same code, it quickly becomes confusing what is what, if you can't
    > distinguish it from their name.


    In my opinion if it does, you are doing something wrong.

    > The code may be very clear as you write it, and even the next day, but
    > return to the code some months or even years later, and you'll often notice
    > yourself repatedly thinking "where was this declared again? Was this a
    > member variable or what?" When you find yourself doing that, it's a clear
    > sign that you really need to start using a naming convention.


    Well I haven't yet after 20 odd years of C++!

    --
    Ian Collins
    Ian Collins, Jun 3, 2012
    #14
  15. Juha Nieminen <> wrote:
    > wrote:
    >> its also handy to label member variables in some way. I've seen
    >>
    >> _member, member_

    >
    > Personally I dislike those because they only add to the obfuscation of
    > the code (especially when mixed with operators), and because the
    > convention is single-purpose (ie. the same convention cannot be used
    > to distinguish from class variables, local variables and variables
    > inside namespaces, named or unnamed).


    At work we use:

    m_member;
    g_global; // also inside namespaces
    s_static;
    local;

    Tobi
    Tobias Müller, Jun 3, 2012
    #15
  16. Bo Persson Guest

    Juha Nieminen skrev 2012-06-03 09:12:
    > Ian Collins<> wrote:
    >> I've never bothered with member variable decoration. It should be clear
    >> from the context what is and what isn't a member.

    >
    > Maybe it should, but it that's quite an impossible demand to make in
    > all cases.
    >
    > When you start mixing function-local variables, member variables, class
    > variables, compilation-unit-local variables and compile-time constants in
    > the same code, it quickly becomes confusing what is what, if you can't
    > distinguish it from their name.


    The you should consider naming the non-members specially, because THEY
    are the odd case. Using members is normal, and doesn't need to be
    specifically marked.


    Bo Persson
    Bo Persson, Jun 3, 2012
    #16
  17. Jorgen Grahn Guest

    On Fri, 2012-06-01, Ian Collins wrote:
    > On 06/ 2/12 11:38 AM, wrote:
    >> Thanks all, i received a good feedback and im willing to start re-
    >> designing this piece of code. There is things that i didnt know and i
    >> just read some of them.
    >> Ill start with the design of the constructor and the initialization
    >> list, here is one of my questions:
    >> i read its good all variables to be in the initialization list, but,
    >> for example, 'lenofFile' i have to use the function
    >> 'CountLinesOfFile(...) to initialize 'lenofFile to it. Can i use
    >> functions inside it and is it good? (I didnt saw any initialization
    >> list do show this, but i read just few tutorials).

    >
    > Another guideline - avoid abbreviations - lengthOfFile is better.


    It's also a name on the wrong abstraction level. It seemed at a quick
    glance not to be the length of some file, but the number of words (or
    lines?) in "words.txt" -- probably the length of the program's
    dictionary. How about "dictionary_size" (or "dictionarySize")?

    Or better yet wrap it into a class of its own so it just becomes
    a size or size() member of the dictionary.

    Good names help you think about the code in terms of the design you
    chose. Bad names hinder that thinking.

    /Jorgen

    --
    // Jorgen Grahn <grahn@ Oo o. . .
    \X/ snipabacken.se> O o .
    Jorgen Grahn, Jun 5, 2012
    #17
  18. Jorgen Grahn Guest

    On Sun, 2012-06-03, Bo Persson wrote:
    > Juha Nieminen skrev 2012-06-03 09:12:
    >> Ian Collins<> wrote:
    >>> I've never bothered with member variable decoration. It should be clear
    >>> from the context what is and what isn't a member.

    >>
    >> Maybe it should, but it that's quite an impossible demand to make in
    >> all cases.
    >>
    >> When you start mixing function-local variables, member variables, class
    >> variables, compilation-unit-local variables and compile-time constants in
    >> the same code, it quickly becomes confusing what is what, if you can't
    >> distinguish it from their name.

    >
    > The you should consider naming the non-members specially, because THEY
    > are the odd case. Using members is normal, and doesn't need to be
    > specifically marked.


    Members /and arguments/ are normal. That's where I see a problem at
    least; I want both to have short and elegant names like "length",
    "other" or whatever, and then I get into the problem Nieminen
    describes.

    I can live with length_ and other_, so that's what I use. But I'm
    aware that there's no consensus.

    /Jorgen

    --
    // Jorgen Grahn <grahn@ Oo o. . .
    \X/ snipabacken.se> O o .
    Jorgen Grahn, Jun 5, 2012
    #18
  19. Guest

    I read its bad to use conio.h, and i use getch() from that header. If
    i have a new project that needs such function, is it good i to write
    it on my own? What would the mass of programmers do?
    , Jun 5, 2012
    #19
  20. Pavel Guest

    Bo Persson wrote:
    > Juha Nieminen skrev 2012-06-03 09:12:
    >> Ian Collins<> wrote:
    >>> I've never bothered with member variable decoration. It should be clear
    >>> from the context what is and what isn't a member.

    >>
    >> Maybe it should, but it that's quite an impossible demand to make in
    >> all cases.
    >>
    >> When you start mixing function-local variables, member variables, class
    >> variables, compilation-unit-local variables and compile-time constants in
    >> the same code, it quickly becomes confusing what is what, if you can't
    >> distinguish it from their name.

    >
    > The you should consider naming the non-members specially, because THEY are the
    > odd case. Using members is normal, and doesn't need to be specifically marked.
    >
    >
    > Bo Persson
    >


    Do you really feel that special marking on 'normal' names somehow denigrates
    them and that on 'odd' names somehow "deservedly punishes" them?

    IMHO people prefer to decorate members for practical rather than educational
    reasons: no one likes extra typing so only the names whose definitions are
    difficult to spot have to be marked as if saying: don't strain your eyes looking
    for my definition here, I am a member, I am defined not here but in the class
    definition.

    I have seen lots of conventions requiring declaration of every name, from very
    respectable teams and firms. Hungarian notation, aThing for parameters vs
    theThing for members are the two coming to mind right off. None stuck for long.

    Members' decorations (sometimes only private members') seem to be staying around
    for longest; I use trailing underscore for privates, feel more or less
    comfortable reading m_members, myMembers etc. but myThing occasionally has
    conflicting connotations (think myTurn); thing_ usually doesn't; m_thing does
    not either but it is more difficult to type and takes more space.

    As for members' 'normalcy'.. how are local variables or parameter names not more
    normal than members? From good software design perspective, of the three
    (locals, parameters and members) a member adds more dependency to the code than
    the other two so if anything, of these three the member is the oddest (and the
    local var is the most normal). Although frankly I have never before thought of
    variables this way ('normal' vs 'odd').

    Just a thought,
    -Pavel
    Pavel, Jun 6, 2012
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. alien2_51
    Replies:
    8
    Views:
    493
    alien2_51
    Jul 16, 2003
  2. Chaddy2222
    Replies:
    15
    Views:
    644
    Chaddy2222
    Jun 6, 2006
  3. =?Utf-8?B?cm9kY2hhcg==?=

    feedback request on design issue

    =?Utf-8?B?cm9kY2hhcg==?=, Sep 8, 2006, in forum: ASP .Net
    Replies:
    2
    Views:
    299
    =?Utf-8?B?cm9kY2hhcg==?=
    Sep 19, 2006
  4. josh
    Replies:
    5
    Views:
    590
    Robert Klemme
    Dec 27, 2011
  5. Tobias Müller

    Re: feedback on code design

    Tobias Müller, Jun 3, 2012, in forum: C++
    Replies:
    0
    Views:
    337
    Tobias Müller
    Jun 3, 2012
Loading...

Share This Page