feedback on code design

[krastavica...@abv.bg]

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;
}
}[/code]

.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[/code]

.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;
}[/code]

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

[nick_keigh...@hotmail.com]

On Wednesday, May 30, 2012 10:52:11 AM UTC+1, krastavica...@abv.bg 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 a setter() (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(){

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.

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

why set to NULL?

> delete[] _hiddenWord;
> _hiddenWord = NULL;
>
>
> delete[] _letterBuf;
> _letterBuf = NULL;
> }

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.

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

getters and setters. These same fairly sensible.

Got bored round about here

<snip>

until...

> //loop until a letter only is pressed
> do {
> inpLetter = getch();
> } while ((inpLetter != 'A' && inpLetter != 'a') &&
> (inpLetter != 'B' && inpLetter != 'b') &&
> (inpLetter != 'C' && inpLetter != 'c') &&

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

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

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

> input = getch();
> switch (input){
> //ENTER KEY -- confirms the letter
> case 13:
> /* save inputed letter in first whitespace element

<snip>

> 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?

what? Put them in the header file?

etc, please
> on everything

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

[krastavica...@abv.bg]

On 30 Май, 15:30, nick_keighley_nos...@hotmail.com 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 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?

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

[Juha Nieminen]

I apologize for not having the patience of going through the entirety of
your code, but I will comment on your constructor/destructor design.

krastavica...@abv.bg wrote:
> .cpp
> [code]#include "HANGMAN.h"
>
> //Ctor and Dtor
> HANGMAN::HANGMAN(){

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

> SetConsoleTitle ("Hangman v1.0");
> hStdout = GetStdHandle (STD_OUTPUT_HANDLE);

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.

> exitGame = false;
> inGame = false;

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

> lenofFile = CountLinesOfFile ("words.txt"); //Count lines
> CreateWordsBuf (lenofFile, "words.txt"); //Allocate and store
> them

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

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

Again something that is dubious to do in a constructor, and looks more like
code that should be in main().

[nick_keigh...@hotmail.com]

On Wednesday, May 30, 2012 9:25:15 PM UTC+1, krastavica...@abv.bg wrote:
> On 30 Май, 15:30, nick_keighley_nos...@hotmail.com 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.

[Tobias Müller]

<krastavica...@abv.bg> 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

[nick_keigh...@hotmail.com]

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

_member, member_ and m_member

[Bruno Beltran]

On Friday, June 1, 2012 3:56:40 AM UTC-5, nick_keigh...@hotmail.com wrote:
> On Thursday, May 31, 2012 5:19:09 PM UTC+1, Tobias Müller wrote:
> > <krastavica...@abv.bg> 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:
> >
>

I agree, unusual. As a beginner, you will be beset on all sides by
naming conventions. I would follow Tobias's *good* advice, and
recommend bookmarking this page
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
for future reference with regards to formatting issues.


Bruno

[Scott Lurndal]

Bruno Beltran <brunob...@gmail.com> writes:
>On Friday, June 1, 2012 3:56:40 AM UTC-5, nick_keigh...@hotmail.com wrote:

>> On Thursday, May 31, 2012 5:19:09 PM UTC+1, Tobias M=FCller wrote:
>> > <krastavica...@abv.bg> wrote:

>> > > I though functions to start with uppercase followed by small ones, an=

>d
>> > > 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 :?

>> >=20

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

>> >=20
>>=20
>
>I agree, unusual. As a beginner, you will be beset on all sides by=20
>naming conventions. I would follow Tobias's *good* advice, and=20

>recommend bookmarking this page
>http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

>for future reference with regards to formatting issues.=20
>

Interesting set of guidelines. There are many different styles of
naming, none of which is superior. However, a particular project or
set of projects should use consistent conventions, and one should avoid
hungarian notation.

Interestingly, using both exceptions and RTTI are prohibited in the
google style guide, and streams are only to be used for logging.

[krastavica...@abv.bg]

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

[Ian Collins]

Another guideline - avoid abbreviations - lengthOfFile is better.

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

--
Ian Collins

[Juha Nieminen]

nick_keigh...@hotmail.com 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).

[nick_keigh...@hotmail.com]

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.

[Ian Collins]

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

[Juha Nieminen]

Ian Collins <ian-...@hotmail.com> 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.

[Ian Collins]

On 06/ 3/12 07:12 PM, Juha Nieminen wrote:
> Ian Collins<ian-...@hotmail.com> 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

[Tobias Müller]

At work we use:

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

Tobi

[Tobias Müller]

Scott Lurndal <sc...@slp53.sl.home> wrote:
> Interesting set of guidelines. There are many different styles of
> naming, none of which is superior. However, a particular project or
> set of projects should use consistent conventions, and one should avoid
> hungarian notation.

The original hungarian notation (not how it is [ab]used by Microsoft) is
actually not such a bad idea in my opinion. It uses a prefix for
information that cannot be expressed with types.
Example:
char* variable; // unclear what it is

// hungarian notation (probably I use the wrong prefixes but the idea
should be clear)
char* szVariable // C string (string, zero-terminated)
char* aVariable // Array of chars

Tobi

[Bo Persson]

Juha Nieminen skrev 2012-06-03 09:12:
> Ian Collins<ian-...@hotmail.com> 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

[Jorgen Grahn]

On Fri, 2012-06-01, Ian Collins wrote:
> On 06/ 2/12 11:38 AM, krastavica...@abv.bg 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]

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.

[krastavica...@abv.bg]

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?

[Robert Wessel]

On Tue, 5 Jun 2012 09:45:18 -0700 (PDT), krastavica...@abv.bg
wrote:

>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?

It's not "bad." So long as it's the appropriate way to accomplish the
task at hand. In general it's better to stick with the standard
functions, so long as they will do the trick for you. The usual
getch() in conio.h does not have a replacement in the standard
library.

That being said. conio.h is not a standard header, and any given
system may, or may not, have such a thing. Likewise, whether ot not
there's a getch() in any given conio.h (and the library support that)
is not generally know. And even if there is a getch(), there's not
guarantee it does the same thing as the one you're using now. So you
may have portability issues to deal with, just as you would with any
non-standard library/system calls.

[Pavel]

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

[krastavica...@abv.bg]

Guys please, i only need answer on, what you do when you need a
function such as getch() or similliar that is part of conio.h or non-
standart header, and you really need it. Would you code similliar
function on your own or what would you do?
Thanks

[Robert Wessel]

On Wed, 6 Jun 2012 07:06:01 -0700 (PDT), krastavica...@abv.bg
wrote:

Please quote properly.

How exactly would you write getch() on your own? And what good would
it do? It requires access to non-standard stuff in order to work.

[Jorgen Grahn]

Please don't:
- jump into a thread with a new, completely unrelated question
- ... without changing the subject
- post the same question many times (this one appeared in at least
one other thread I'm reading)

[nick_keigh...@hotmail.com]

if you need it then you use conio.h. I'd probably hide it behind some OS specific library. Maybe even rename it. Hopefully then moving to another would only require changes in a few places.

[Nobody]

Use the curses API. There are implementations for all of the common
platforms. Unix versions use control codes and escape sequences from the
terminfo database, the Windows version uses the Windows console API. You
can use a Unix version on Windows if you want to talk to a terminal or
terminal emulator (either via serial or network) rather than a Windows
console.