feedback on code design

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

  1. 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

    Code (Text):
    #include "HANGMAN.h"

    //Ctor and Dtor

    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

    srand (time(NULL)); //seed for picking element index from


    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
    case 1:
    cout << "Don't let your user see the word's from the text

    //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);


    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, ' ');
    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;
    cout << "Failed to open words.txt for CreateWordBuf (int,
    const char*)" << endl;

    void HANGMAN::Pull_randWord (int filelen){ //Pulls a word from
    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
    _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;

    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);
    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


    //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]){
    //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
    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
    letterInpCoords.X += i; //take value of i + 14 for X
    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
    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
    for (int i = 0; i < lenofLetterB; i++){
    if (_letterBuf[i] == ' '){
    _letterBuf[i] = inpLetter;
    cout << ",";
    letterConfirmed = true;
    break; //case ENTER: ends

    //BACKSPACE KEY -- re-printing letters/visualy
    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,
    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,
    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
    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){
    Code (Text):
    #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
    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
    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

    //windows predef data types
    HANDLE hStdout; //std output handle

    int triesLeft;
    bool exitGame; //flag for the main loop
    bool inGame;   //flag for inside game loop

    char input;     //UI input
    char inpLetter; //input only for letters
    char* _letterBuf;
    int lenofLetterB; //lenth of _letterBuf

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

    char* _hiddenWord;
    string randWord; //Pull_randWord returns this variable
    int lenofRW;     //lenth of randWord

    #endif // HANGMAN_H
    Code (Text):
    #include "HANGMAN.h"

    using namespace std;

    int main()

    while (!HANGM.Get_exitGame()){

    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
    switch (HANGM.Get_input()){

    //New Game -- Enter Key
    case 13:

    //Exit Game -- Esc Key
    case 27:
    HANGM.Set_exitGame (true);


    } //switch (input) ends

    } //while (ENTER && ESC) ends

    //Start Game loop
    while (HANGM.Get_inGame()){




    } //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
    krastavicakrastavica, May 30, 2012
    1. Advertisements

  2. krastavicakrastavica

    Guest Guest

    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 usually like to look at the class declaration first.
    These are the member variables

    //windows predef data types
    good design would separate Windows stuff from application stuff.

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

    why a char* ?
    just use
    Guest, May 30, 2012
    1. Advertisements

  3. where to put them?
    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
    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 :?
    What do you mean, please explain some more here
    can you also explain a bit more on this one, sir.
    Thanks for this one, will read some more about those operators, maybe
    you have a good link in hand?
    nice one. thanks
    can you explain me some more on this one please.
    ER KEY -- confirms the letter
    i mean, can you code a hangman game only seeing the prototype
    functions? With no acces to the .cpp file
    Thanks alot sir, great feedback, learned alot, dont stop if possible
    krastavicakrastavica, May 30, 2012
  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.

    Juha Nieminen, May 31, 2012
  5. krastavicakrastavica

    Guest Guest

    Have you read juha's response? some of what we say overlaps
    perhaps another class?
    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
    um. read this:-

    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
    Juha had a suggestion.
    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

    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.
    Have you access to a good text book? I've always liked Stroustrup, "Accelerated C++" is well thought of.
    Guest, May 31, 2012
  6. Those are at least unusual naming conventions, i would rather use:

    - 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

    Tobias Müller, May 31, 2012
  7. krastavicakrastavica

    Guest Guest

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

    _member, member_ and m_member
    Guest, Jun 1, 2012
  8. 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).
    krastavicakrastavica, Jun 2, 2012
  9. krastavicakrastavica

    Ian Collins Guest

    Another guideline - avoid abbreviations - lengthOfFile is better.

    Yes, you can call functions, other than virtual ones, anywhere in a
    Ian Collins, Jun 2, 2012
  10. 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
  11. krastavicakrastavica

    Guest Guest

    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.
    Guest, Jun 2, 2012
  12. krastavicakrastavica

    Ian Collins Guest

    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, Jun 2, 2012
  13. 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.
    The clarity of code should not depend on having an editor clarifying it
    for you.
    Juha Nieminen, Jun 3, 2012
  14. krastavicakrastavica

    Ian Collins Guest

    In my opinion if it does, you are doing something wrong.
    Well I haven't yet after 20 odd years of C++!
    Ian Collins, Jun 3, 2012
  15. At work we use:

    g_global; // also inside namespaces

    Tobias Müller, Jun 3, 2012
  16. krastavicakrastavica

    Bo Persson Guest

    Juha Nieminen skrev 2012-06-03 09:12:
    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
  17. krastavicakrastavica

    Jorgen Grahn Guest

    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 Grahn, Jun 5, 2012
  18. krastavicakrastavica

    Jorgen Grahn Guest

    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

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

    Jorgen Grahn, Jun 5, 2012
  19. 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?
    krastavicakrastavica, Jun 5, 2012
  20. krastavicakrastavica

    Pavel Guest

    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

    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, Jun 6, 2012
    1. Advertisements

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.