New Member (w/ Code)

I have been searching the forums for a while now and I love how much experience you guys have. Most of you seem very knowledgeable and very helpful so I decided to sign up.

I've recently taken up programming again after about 5 years and I can't believe it's taken me this long, I missed it so much.

Anyways, I've been working on some console programs and been trying to learn some new things. Here is a sample of my current code that I'm working on. It's a header file for creating a Menu class. Please give me input on it and I'm not afraid of you being critical. Let me know what you think.

http://codepad.org/doqGWgPV

Disch (13742)

Welcome to the forums!

It's a little difficult to give commentary on the functionality of the class without the full context. I'm not sure what the left/right selectors are for.

You look like you are making a good effort to keep the code organized and clean, which is very important. You're also keeping things encapsulated nicely. Very good job.

However I do see some problems:

1) Why is the right selector always an empty string? What's the point of passing a parameters to set it in your constructor/Selector functions if the parameter goes unused?

2) Your logic in the Options function is backwards:

	if(vMenuOptions.size() <= position)  // if the size of your vector is LESS than the position...
		vMenuOptions[(position - 1)] = option; // then this will be out of bounds
	else if(vMenuOptions.size() > (position + 1)) // if the size is greater..
		for(unsigned int i = vMenuOptions.size(); i < position; i ++) // then you proceed to make it even bigger!
			vMenuOptions.push_back("");
	if(vMenuOptions.size() == (position - 1))
		vMenuOptions.push_back(option);

I think you mixed up < and > there.

Also, continuously push_back'ing a value is a bad way to resize a vector. It would make much more sense to simply call resize(). This function can be simplified greatly:

if(vMenuOptions.size() <= position)  // if vector isn't big enough
  vMenuOptions.resize( position + 1, "" );  // make it big enough

vMenuOptions[position] = option;  // set appropriate entry

3) You can't put function bodies in header files like that unless they are inlined or templated. The functions inside the class are okay (if you give the body inside the class, it is implicitly inlined), but that last Options function will cause problems if this header file is #included in multiple cpp files. Move that function body to a .cpp file -- or make it inline.

4) vector is in the std namespace. I'm not sure how this code is compiling for you, but you should be getting an error saying that vector is undefined. You should have
std::vector<const char*> vMenuOptions;

5) Consider using strings instead of char pointers. This way you avoid memory ownership issues.

Other stylistic things that aren't so important:

1) avoid using #define to define constants. Really, avoid #define as much as possible. The only thing you'll really need it for (usually) is the header guard. #defines ignore language scope rules and pollute the namespace, making them error prone. Constants should be declared with the const keyword... or possibly with an enum:

class Menu:
  public:
    enum
    {
      Enter = 13,
      Esc = 27,
      ...
    };

2) I'm not a fan of hungarian notation. There's no need to prefix the type of the variable in the name. It doesn't really do anything to benefit you and just makes maintanance harder (what if you decide to change the type later? Do you go back and rename the variable in all the spots you used it?)

3) Also, "Option" could probably be changed to a more descriptive name. I would probably call them "AddOption" and "SetOption", as that is more descriptive of what they actually do.

Last edited on

Volatile Pulse (1543)

1) Why is the right selector always an empty string? What's the point of passing a parameters to set it in your constructor/Selector functions if the parameter goes unused?

Because I was going to skip it until I was finished. Has been changed since I posted the code though.

2) Your logic in the Options function is backwards:

Lol I noticed last night when I started working on it again. Fixed as well.

Also, continuously push_back'ing a value is a bad way to resize a vector. It would make much more sense to simply call resize(). This function can be simplified greatly:

I knew about resize(), but I was really tired and making programming hard. Code fixed.

I never knew this, can you explain into more detail as to why functions can't be in the header? I never had compile errors or warnings that I was aware of and I just made a header file that has functions and still no warnings. or is it just bad practice?

4) vector is in the std namespace. I'm not sure how this code is compiling for you, but you should be getting an error saying that vector is undefined. You should have
std::vector<const char*> vMenuOptions;

I'm not sure why either. But fixed regardless.

5) Consider using strings instead of char pointers. This way you avoid memory ownership issues.

I like the idea of the char pointers. I find char* is easier to work with in this situation, but I will get back to changing them, gives me something else to learn about.

1) avoid using #define to define constants. Really, avoid #define as much as possible. The only thing you'll really need it for (usually) is the header guard. #defines ignore language scope rules and pollute the namespace, making them error prone. Constants should be declared with the const keyword... or possibly with an enum:

I haven't changed it yet, but that is definitely something I will do. I didn't use enums much in school bc I saw no point, but I will definitely get that changed.

I like the idea of naming the variables after what it is, and I think it's easier for me to name them this way. I was taught it in high school, and I have always found it hard to break, especially seeing a lot of windows api books referring to it. And yes, I actually do search/replace when I change my variable types. I'm OCD about that kind of stuff. I also know it takes a few more keystrokes, but with autocomplete, it's not a big deal. I've also seen the arguements about it and nothing applies to me yet.

3) Also, "Option" could probably be changed to a more descriptive name. I would probably call them "AddOption" and "SetOption", as that is more descriptive of what they actually do.

Again, I was tired and anxious to see feed back so I posted without thinking about my functions first. Fixed.

I appreciate the feedback, and you have given me a bunch of things to look into once I'm satisfied with how far I've gotten on this and some other files. I'm going to post my code again, and I know I'm going to hear about the system() functions, but they're purely designed for a windows environment at the moment. I've seen Duoas getting frustrated with others, and I will hopefully get around to learning about other OS's, in the meantime, the code works for me, and I believe a majority of other windows users.

Main.cpp

#include <iostream>

#include "menu.h"
#include "header.h"

int main() {
	VP_SetCursor(false);
	Menu MainMenu("->","<-");
	MainMenu.AddOption("Single Player");
	MainMenu.AddOption("MultiPlayer");
	MainMenu.AddOption("Exit");
	while(MainMenu.Play() != 3) {};

	return 0;
}

Menu.cpp
http://codepad.org/iNCVzUZj

Header.cpp

#ifndef _VP_HEADER_H_
#define _VP_HEADER_H_

#include <windows.h>
#include <conio.h>

void VP_SetCursor(bool bCursor) {
	HANDLE hConsole = GetStdHandle(STD_OUTPUT_HANDLE);
	CONSOLE_CURSOR_INFO curCursorInfo;
	curCursorInfo.bVisible = (bCursor ? TRUE : FALSE);
	curCursorInfo.dwSize = 1;
	SetConsoleCursorInfo(hConsole, &curCursorInfo);
}

void VP_GoToXY(int x, int y) {
	HANDLE console = GetStdHandle(STD_OUTPUT_HANDLE);
	COORD CursorPosition;
	CursorPosition.X = x;
	CursorPosition.Y = y;
	SetConsoleCursorPosition(console, CursorPosition);
}

void VP_ClearScreen() { system("cls"); }

#endif

Any more opinions on either the new or old code? It works well, and I always try to make it dummy proof, but any improvements you can see or any other criticism would be greatly appreciated. Thanks again for all of the help and pointers.

Last edited on

cire (8284)

No, it's not just bad practice. Problems aren't encountered until you include the header in more than one .cpp file. When you do, multiple definitions for the same function are generated, which violates the One-Definition-Rule and which generally causes your linker to vomit error messages in your general direction.

I'm going to post my code again, and I know I'm going to hear about the system() functions, but they're purely designed for a windows environment at the moment.

system is bad regardless of what environment code is designed for.

http://www.cplusplus.com/forum/beginner/1988/3/#msg10830

Volatile Pulse (1543)

I'm confused on this still. I don't understand what makes it an issue. I mean I'm used to what I was taught and I am stubborn, but it's not to say that I can't, or even won't, break bad habits, but I want to know what makes it a bad habit. For example, if I declare all my functions in a header, what would be the need of any other CPP files, but since my header file has a header guard in it, doesn't that prevent the violation of One-Definition-Rule? And if I remember correctly, including a library or header file in your program will just copy the code from that header file into where you #included it, am I wrong? What makes copying the info from the header file into your other files so much worse than forcing the compiler to look up yet another file?

system is bad regardless of what environment code is designed for.

I don't disagree with anyone on this, in regards to portability, but other than portability and being non compliant of the standard library, what is wrong with it? The idea behind my few system and getch() functions is to manipulate the console on Windows systems, and Windows only. Like I said earlier, I will get around to switching everything over one day, but I'm trying to relearn a lot of my old syntax and previous knowledge on C++. I guess I'm looking for more of a, "What is a better way of doing this?" answer. I read through every post here:
http://www.daniweb.com/software-development/cpp/threads/148611/how-to-identify-which-os-i-am-using#post703879
and needless to say, I don't like using code that I don't understand. I understand what each thing does, but I like to learn everything before I use it. For instance, I have no clue how to program differently for linux systems, and while I should have a fail safe for linux, anyone who is programming with my code should be able to switch those few things around. Even worse, Apple is getting very big now and I have yet to see anything regarding Apple computers, or are they linux based systems as well?

Sorry for the long winded replies, but I would like to learn more about what is being done "wrong" instead of just being told it's wrong.

cire (8284)

For example, if I declare all my functions in a header, what would be the need of any other CPP files, but since my header file has a header guard in it, doesn't that prevent the violation of One-Definition-Rule?

There's nothing wrong with declaring functions in headers, indeed one would expect you to. It is supplying definitions of non-inline functions in headers that is a problem.

but since my header file has a header guard in it, doesn't that prevent the violation of One-Definition-Rule?

No. Header guards are to deal with multiple inclusions of the same headers in the same translation unit (which, for all practical purposes is one .cpp file.) including a header in one .cpp file has no effect on a different .cpp file.

And if I remember correctly, including a library or header file in your program will just copy the code from that header file into where you #included it, am I wrong?

That is correct. And that is why you don't want definitions of non-inline functions in header files.

Say you have a function called strlen that determines the length of a c-string supplied to it via argument. The code isn't very long so you just leave the function definition in a header file with related functions. You have two .cpp files in your project. You're doing some string processing in both of them, so you #include that header file in both .cpp files.

An #include copies the entire text of the .h file into the .cpp where you included it, so both .cpp files have their own separate copy of the strlen function. The compiler emits code for each separate translation unit and in each .obj (or whatever extension your compiler uses for object code) file there exists a strlen function. Now the linker starts linking code together and it finds references to the strlen function... but which strlen function does it use? It can't know. There's two definitions for the same function which is a violation of the ODR, and you, the programmer, are supposed to ensure that doesn't happen, so it vomits errors at you.

I don't like using code that I don't understand.

Then you should definitely dislike using system calls, because you have absolutely no control over what may happen when you expose yourself like that. Suppose someone on here were to write a piece of malicious code (that you compiled and ran as an example that illustrated something) that placed a file.. say cls.bat somewhere on your system that actually reformatted your hard drive.

And then you used a system("cls") in your code that, instead of invoking what you were expecting, invoked the batch file that reformatted your hard drive... Oops.

Volatile Pulse (1543)

I get what you're say about it now, thank you for explaining it to me into more detail. I don't typically have, correction, I've never had more than one cpp file in a project at any given time. What would be the purpose? Collaboration?

And I do agree that system() calls are bad practice, I've known that for a while, but I haven't learned how to clear the console properly yet, and I can only absorb so much at a given time. What I know about system() is why I use it, it takes the string and runs it through the Console. system("cls") will always clear the screen from what I learned about DOS. a call to the batch file would have to be cls.bat, would it not? I'm not trying to defend it, just trying to grasp everything at once.

Late nights with no caffeine makes the learning process a lot harder. Also, I will work on converting my Header.h file into a Header.cpp as well. Is it a bad idea to link every header I MIGHT use in header.h, or only ones that are nessecary. Is cluttering .cpp files with #includes a bad thing since they have header guards? More specifically, I use #include<iostream> in every app, as I believe most people do, is it wise to declare iostream, windows, menu, and whatever headers I use often in my header.h/.cpp files?

Volatile Pulse (1543)

I did some more research on the multiple .cpp files in a project, and I understand it a lot better. But about my other question about declaring all the possible header files I might need in a header.h file, is this a bad idea? Should I only declare header files while I need them and only define the header files I need per other file?

menu.h

1
2
3

#include<iostream>
#include<vector>
#include<string.h>

header.h

#include<iostream>
#include<vector>
#include<string.h>
#include<windows.h>
#include<stdlib>
#include"class1.h"
#include"class2.h"
#include"menu.h"

Declaring everything in header.h is not a good idea, but prevents having to declare all of the includes in other files when I can just use header.h. Thoughts?

Disch (13742)

For small projects having a single "include everything" header is fine.

For larger projects it's bad because it murders compile times. Note that every file you #include has to be compiled in every cpp file that includes it. So if you have 20 headers, and each is included in 10 cpp files, that means you are compiling (20+1)*10 = 210 files when you build the project.

EDIT:

That said... even in large projects it's common to have "groups" of headers bundled into a single .h file.

Last edited on

Volatile Pulse (1543)

So stick to only including the header files that are absolutely required to run the code and nothing more? I've already been working on switch everything over. Better to catch it early than later. Thanks guys.

Disch (13742)

If you are already including everything in every cpp file, don't bother changing it unless your program takes really long to build.

The only issue here is compile speed. And if your program is compiling fast then there's no point in doing extra work to change it. It will not make any difference in the final program.

Volatile Pulse (1543)

Thanks Disch. But I have recently run into a new problem with my menu header. The code is posted here: http://codepad.org/qRoaqVko

Specifically during the process of converting from the char* to the strings, the strings aren't working correctly now, at first I had an issue with just the title string, but now I'm having issues with all of the strings copying over except the title string. Checking the string values in each class function as they get assigned returns the correct value, but when I get to Play, all of those strings are empty except for my vector of strings.

I tried pointers, thinking that maybe the values were getting lost in the confusion, but that just threw some errors/warnings at me.

Also, any snippets that you think need worked on would be greatly appreciated.

[Edit] I figured it out, I didn't know that you couldn't call other constructors from the class, it didn't give me an error or warning...unless, is there a way to do this:

       Menu() {
           Menu("", "", "");
       }

       // Define Empty Menu and set Left and Right Selectors
       Menu(std::string leftSelector, std::string rightSelector) {
           Menu("", leftSelector, rightSelector);
       }

       // Define Empty Menu with a Title and set Left and Right Selectors
       Menu(std::string title, std::string leftSelector, std::string rightSelector) {
           SetSelectors(leftSelector, rightSelector);
       }

Last edited on

Topic archived. No new replies allowed.

New Member (w/ Code)

C++

Forum