new programmer could use some code critiqued 1

[benjamin17801]

I'm posting this message to try to get some help an pointers about programming. I'm new to c++ and object oriented programming. The code I posted is an idea i'm working on for a directx driven gui. I'm using an article on GameDev.net by Mason McCuskey. Any comments anything at all that is constuctive would be nice. Thank you and it is appreciated. Here is the little bit of code I have so far:

this is the header file:

class Xg_Window
{

int TopLeft_X; // 0 - 9600 virtual window coords for location
int TopLeft_Y; // 0 - 7200 " "

int SizeOf_X; // Width in virtual windows coords
int SizeOf_Y; // Heighth " "

int BkgColor; // Back ground color of window
int BrdColor; // Border color of window
int Opacity; // 0 - 100 value to indicate the opacity of the window


// Private Xg_Window management code
// Called in the constructer

void Add_Window(); // A pointer to a Xg_Window Object to be added to a collection at the top


public:

Xg_Window( int, int, int, int, int, int, int ); // Sets the initial attributes for the Xg_window object
~Xg_Window(); // Destroys the object

// Top physical attributes
void New_Location( int, int ); // Sets new TopLeft_X and the TopLeft_Y fields
void New_Size( int, int ); // Sets new SizeOf_X and the SizeOf_Y fields
void New_Color( int, int, int ); // Sets new BkgColor, BrdColor, Opacity fields

// public Xg_Windows management code

static void Delete_Window(Xg_Window *w); // A pointer to a Xg_Window Object to be deleted from the container
static void BringTo_Top( Xg_Window *w ); // Brings The Xg_Window Object that is pointed to, to the top
};




and the .cpp file


#include <stdafx.h>
#include <Gui.h>
#include <vector>
#include <algorithm>

using namespace std;

vector<Xg_Window*> vi;
vector<Xg_Window*>::iterator i;

// Constructer and Destructer for the Xg_Window object
// constucter sets the initial size location and color attribute

Xg_Window::Xg_Window(int TL_X, int TL_Y, int SO_X, int SO_Y, int BKG_C, int BRD_C, int OP)
{
TopLeft_X = TL_X;
TopLeft_Y = TL_Y;
SizeOf_X = SO_X;
SizeOf_Y = SO_Y;
BkgColor = BKG_C;
BrdColor = BRD_C;
Opacity = OP;

Add_Window();
}

Xg_Window::~Xg_Window()
{
}



// Windows physical attributes, these member functions of class Xg_Window
// set the new private data fields for the class

void Xg_Window::New_Location(int LX, int LY) // Sets the top left x and y of the windows location
{
TopLeft_X = LX;
TopLeft_Y = LY;
}

void Xg_Window::New_Size(int SX, int SY) // Sets the size of the window
{
SizeOf_X = SX;
SizeOf_Y = SY;
}

void Xg_Window::New_Color(int BK, int BD, int OP) // Sets the background and border color for the window and the transparency
{
BkgColor = BK;
BrdColor = BD;
Opacity = OP;
}


// Windows Management code Add_Window is a private members of Xg_Window called upon construction
// and destruction of Xg_window object

void Xg_Window::Add_Window()
{
vi.push_back(this); // Push a pointer to this window into the collection vi
}

void Xg_Window:elete_Window(Xg_Window *w)
{
i = find(vi.begin(), vi.end(), w); // Check to see where the poniter to the Xg_Window object is in the vector
vi.erase(i); // then deletes it
delete w;
}

void Xg_Window::BringTo_Top(Xg_Window *w)
{
Xg_Window *p = w; // copies the object
i = find(vi.begin(), vi.end(), w); // finds the object in the vector
vi.erase(i); // deletes the object from its position
vi.push_back(p); // pushes the copy back on the bottom( which is the top for me )
}

 

[timmay3141]

Using underscores in class/function/variable names is not looked favorably upon except for Hungarian prefixes (it was standard when STL came out, but not any more). Also, I strongly recommend using an m_ prefix for variables that are members of a class, a g_ prefix for globals, and s_ prefix for static member variables. The rest of Hungarian notation is up to you, but m_ and g_ help immensly in following code IMO. If you don't know what Hungarian notation is, look it up. I personally find m_, g_, s_ useful, as well as p for pointers, and occasionally other times where I have two variables that are for similar things (and have similar names) but have different types. In short, just use XgWindow instead of Xg_Window(though maybe rename that), SizeOf_X and Y change to m_SizeX and m_SizeY. I would also recommend putting the parameter names in the declarations of functions; although this is not required, there is really no reason not to, and it helps when someone else looks at your code, and if you ever forget which parameter goes where, Intellisense will tell you if you have the variable names in the declaration. You are using a couple global variables - this should generally be avoided if possible in favor of static member variables. There are times when global variables are good, but in this case, they are used entirely by your class, so they should be static members.

Good luck with your GUI, I wrote one myself recently in DirectX, and if you need any advice on how to do something just ask.

 

[benjamin17801]

timmay3141, Thank you for the response I'm going to make them changes you recommended. And I'm going to read more about hungarian notation. thanks again

 

[cpjust]

Another thing is you should not use "using namespace std;", instead put "using std::vector;"...
Also, some of the variable names don't look very descriptive to me. Variable names (as well as function names) should be very descriptive as to what their function is, and they should always start with a small letter and use a capital for the beginning of each new word in the name.
ex. int accountBalance = 0;
char* pszAccountName = NULL; // psz is Hungarian Notation for pointer to a null terminated string.

I also like to start all of my public functions with a capital letter and all private functions with small letters to make them easier to recognize.

 

[Salem]

vector<Xg_Window*> vi;
This should be a static member of the class

vector<Xg_Window*>::iterator i;
This should be a local variable inside the functions which specifically need an iterator.

I think I would prefer to see some additional typedefs

typedef int coord_t;             // anywhere in the coordinate plane
typedef unsigned long colour_t;  // as 8-bit RGB
typedef unsigned char opaque_t;  // a percentage

Then you can go on to do this

coord_t TopLeft_X; // 0 - 9600 virtual window coords for location
coord_t TopLeft_Y; // 0 - 7200 " "

coord_t SizeOf_X; // Width in virtual windows coords
coord_t SizeOf_Y; // Heighth " "

colour_t BkgColor; // Back ground color of window
colour_t BrdColor; // Border color of window
opaque_t Opacity; // 0 - 100 value to indicate the opacity of the window

Some compilers (and code checking tools like pc-lint) will then be able to check to make sure you don't do dumb things like [tt]Opacity = BrdColor;[/tt] without generating a warning. Something an amorphous block of ints could not do.

--

 

[dave05]

I agree with the Hungarian notation notes..
m_ g_ s_. I'd additonally throw in a p_ for values that you pass in as parameters to the function.