General Improvments

[Quell]

Hello.
I have been coding for a while but i have pathetic conventions and style.
Here is a sample piece of code that i ahve and i was wondering if anyone got any ideas on how i could improve.Any critisims+ideas are welcome.

#ifndef sys_console
#define sys_console
#include <windows.h>
#include <iostream>
#include <conio.h>
static const WORD fgBlack= 0;
static const WORD fgLoRed= FOREGROUND_RED;
static const WORD fgLoGreen= FOREGROUND_GREEN;
static const WORD fgLoBlue= FOREGROUND_BLUE;
static const WORD fgLoCyan= fgLoGreen|fgLoBlue;
static const WORD fgLoMagenta= fgLoRed|fgLoBlue;
static const WORD fgLoYellow= fgLoRed|fgLoGreen;
static const WORD fgLoWhite= fgLoRed|fgLoGreen|fgLoBlue;
static const WORD fgGray= fgBlack|FOREGROUND_INTENSITY;
static const WORD fgHiWhite= fgLoWhite|FOREGROUND_INTENSITY;
static const WORD fgHiBlue= fgLoBlue|FOREGROUND_INTENSITY;
static const WORD fgHiGreen= fgLoGreen| FOREGROUND_INTENSITY;
static const WORD fgHiRed= fgLoRed|FOREGROUND_INTENSITY;
static const WORD fgHiCyan= fgLoCyan|FOREGROUND_INTENSITY;
static const WORD fgHiMagenta= fgLoMagenta|FOREGROUND_INTENSITY;
static const WORD fgHiYellow= fgLoYellow|FOREGROUND_INTENSITY;
static const WORD bgBlack= 0;
static const WORD bgLoRed= BACKGROUND_RED;
static const WORD bgLoGreen= BACKGROUND_GREEN;
static const WORD bgLoBlue= BACKGROUND_BLUE;
static const WORD bgLoCyan= bgLoGreen|bgLoBlue;
static const WORD bgLoMagenta= bgLoRed|bgLoBlue;
static const WORD bgLoYellow= bgLoRed|bgLoGreen;
static const WORD bgLoWhite= bgLoRed|bgLoGreen|bgLoBlue;
static const WORD bgGray= bgBlack|BACKGROUND_INTENSITY;
static const WORD bgHiWhite= bgLoWhite|BACKGROUND_INTENSITY;
static const WORD bgHiBlue= bgLoBlue|BACKGROUND_INTENSITY;
static const WORD bgHiGreen= bgLoGreen|BACKGROUND_INTENSITY;
static const WORD bgHiRed= bgLoRed|BACKGROUND_INTENSITY;
static const WORD bgHiCyan= bgLoCyan|BACKGROUND_INTENSITY;
static const WORD bgHiMagenta= bgLoMagenta|BACKGROUND_INTENSITY;
static const WORD bgHiYellow= bgLoYellow|BACKGROUND_INTENSITY;
class csys_console{
public:
csys_console();
~csys_console();
void cls();
void color(WORD wColor);
void pause();
void cursor(DWORD dwSize,bool bVisible);
};
#endif

#include "sys_console.h"
csys_console::csys_console(){
}
csys_console::~csys_console(){
}
void csys_console::cls(){
COORD coordScreen = {0,0};
DWORD cCharsWritten=0;
CONSOLE_SCREEN_BUFFER_INFO csbiWindow;
DWORD dwConSize=0;
if(!GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &csbiWindow))std::cout<<"Error getting screen buffer info."<<std::endl;
dwConSize = csbiWindow.dwSize.X * csbiWindow.dwSize.Y;
if(!FillConsoleOutputCharacter(GetStdHandle(STD_OUTPUT_HANDLE), (TCHAR) ' ', dwConSize, coordScreen, &cCharsWritten ))std::cout<<"Error clearing the console."<<std::endl;
if(!FillConsoleOutputAttribute(GetStdHandle(STD_OUTPUT_HANDLE), csbiWindow.wAttributes,dwConSize, coordScreen, &cCharsWritten ))std::cout<<"Error clearing the console."<<std::endl;
if(!SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE), coordScreen ))std::cout<<"Error reseting the cursor position."<<std::endl;
}
void csys_console::color(WORD wColor){
if(!SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE),wColor))std::cout<<"Error setting new color."<<std::endl;
}
void csys_console::pause(){
std::cout<<"Press any key to continue."<<std::endl;
while(!_kbhit()){
Sleep(100);
}
_getch();
}
void csys_console::cursor(DWORD dwSize,bool bVisible){
_CONSOLE_CURSOR_INFO cCursor;
cCursor.dwSize=dwSize;
cCursor.bVisible=bVisible;
SetConsoleCursorInfo(GetStdHandle(STD_OUTPUT_HANDLE),&cCursor);

}


Any ideas how i could improve the code?
Note that the current code works.
Thx alot.

[exterminator]

Well, as for advices (if the code really serves your purpose):
1. I would not use static const.. rather I would group all the constants into a seperate namespace called CommonConstants.. or whatever sounds nicer.
2. Keep the constants into a different file seperate from the class definition.
3. Would write if-else-.. statements like this:if (condition){
//set of statements
else{
//another set of statements
}rather than putting all in one line (even when there is no else).
4. Put a using std::cout and using std::endl at the top so that I don't need to write std::blah all the time.
5. Some inlined comments about what some part of the code do..
6. Most importantly - there is no exception handling in your code. I would not let it go that easy without it.

Hope this helps. Regards.

[Graham]

Use blank lines to isolate "atomic" sections of code. As exterminator says, put control statements on their own line, and the controlled code on lines following them (indented), and avoid "statics" - if they're meant to be local to the file, put them in an unnamed namespace.

DITCH HUNGARIAN NOTATION - it adds nothing and often ends up lying; rather prefer to give variables and functions names that clearly express their purpose.

Let's have a look at the function cls(). First, reformatted with the hungarian warts removed:

void csys_console::cls()
{
COORD Screen = {0,0}; // "Screen"?
DWORD CharsWritten=0;
CONSOLE_SCREEN_BUFFER_INFO Window; // "Window"?
DWORD ConSize=0; // "ConSize"?

if(!GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &Window))
std::cout<<"Error getting screen buffer info."<<std::endl;

ConSize = Window.dwSize.X * Window.dwSize.Y;

if (!FillConsoleOutputCharacter(GetStdHandle(STD_OUTPUT_HANDLE),
(TCHAR) ' ', ConSize, Screen, &CharsWritten ))
std::cout<<"Error clearing the console."<<std::endl;

if (!FillConsoleOutputAttribute(GetStdHandle(STD_OUTPUT_HANDLE),
Window.wAttributes, ConSize, Screen, &CharsWritten))
std::cout<<"Error clearing the console."<<std::endl;

if (!SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE), Screen ))
std::cout<<"Error reseting the cursor position."<<std::endl;
}

(For some reason, the "code" formatting adds extra space to the indents - I had intended for each indent to be four spaces.)

Notice the declarations where I've queried the name of the variable. Ask yourself: does the name really tell you what the purpose of that variable is? If someone else had written this, and you were picking it up for the first time, would you be thanking the original author for making your life easier?

Finally (and again pointed out by exterminator) you are printing an error message and then carrying on with the processing. Surely some mistake?

[Quell]

yeah, exception handling needs improvment.
thx alot for that stuff.
If anyone gets any ohter ideas plz post:D

[NMTop40]

Remember that your functions are called by other functions. Functions can't see cout statements output by your functions so would have no idea that an error has occurred.

You might consider using enums for constant integral values.

Don't put unnecessary includes in header files. Some of your constants might come from windows.h so you need that (WORD you can do away with) but you didn't need the conio.h or iostream in that header.