Public Group

# Ideas about ehre I am at learning C++

This topic is 3632 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

##### Share on other sites
Well, to be honest, you didn't use much of C++ apart from std::cout and std::string. You use pointers where references sould be used and you use macros.

##### Share on other sites
It's a good effort at commenting and writing readable code (though I wouldn't create such long lines, it's distracting having to scroll your editor window just to read everything). The style is also relatively consistent throughout the file.

Judging your C++ knowledge, however, I'd say you're on a good 1 out of 10 (there's just so much about C++, if you rate someone quite fit with the C++ Standard Library who properly uses exceptions and templates as a 6 or 7, then that's clearly a 1).

Assuming you're using that style throughout your projects (and not only when you write code for other people to look at ;)) and you're ready to adjust it as you learn new stuff, I'd say you're on the right path. Keep up the good work!

##### Share on other sites
Quote:
 Original post by Appmonsterhttp://tommagson.pastebin.com/f1ef9da51 <--- Link to paste-bin to see my program.
You can use the source tags to include code in your post.

Note that I haven't looked at what the code does or whether it is properly designed.

#define GOODMSG GOOD_MSG

Why is this?

using namespace std;

Although not apparent here, importing a namespace globally may lead to problems in more complex projects with lots of includes.

//Declear maximum range of search...

I think you mean declare. [smile]

int errorStatus = 0;     //0 = No error, 1 = Range Error, 2 = Fatal Loop Error

Good candidate for an enumeration:
enum ERROR_STATUS{  ERR_NO_ERROR,  ERR_OUT_OF_RANGE,  ERR_FATAL_LOOP};
Same goes for the text color constants.

int    errorStatus = 0;     //0 = No error, 1 = Range Error, 2 = Fatal Loop Errorint    setColor    = 0;     //Holds a color codeint    temp        = 0;    //Temporary storage for inputint    iRun        = 0;   //Counting Variable used for making decisions on the stability of the loop.bool   appRunning  = true;//Simle boolean variable controling the execution of the app.string error       = "null";//Holds the error message to be passed.

In C++, you are not required to declare all of you're variables at the top of a function. It's a matter of style perhaps, but you might want to declare them where they are first used.

int showUpTo(int max, string* err,int* errCode)

Why not use references here?
int showUpTo(int max, string& err,int& errCode)

Happy learning! [smile]

##### Share on other sites
Cygon
Quote:
 (and not only when you write code for other people to look at ;))

Lol, nope I write my code like that pretty much all the time (sometimes I write messy stuff, then go back and make it clearer, but i stick to that format)
WanMaster
Quote:
 You can use the source tags to include code in your post.

I did consider that (its like te only tag I know lol) but I decided it was too large and used pastebin instead.

Quote:
 #define GOODMSG GOOD_MSG

I used that because I was lazy and couldn't be bothered finding the instance of GOODMSG to change it ;p

Quote:
 Although not apparent here, importing a namespace globally may lead to problems in more complex projects with lots of includes.

I did not know that, the only way i know how to make myself stop having to write std:: before everything is by using that, is there another way I could do it?

Quote:
 Good candidate for an enumeration:

Yes, good point for the error messages, but for the text colors mabey a different story.
As I understand enumerations kinda assign a Name to a number, starting at one. The text colors are many different numbers (ie 30,14 ect) so im not sure if that would work with the colours.

Quote:
 In C++, you are not required to declare all of you're variables at the top of a function. It's a matter of style perhaps, but you might want to declare them where they are first used.

I do like to declare (=P) my variables at the top (grouped by type if you noticed ;P) just because I know where I can find them. If I was to declare them before I use them would it save memory for a little while?

Quote:
 Why not use references here?int showUpTo(int max, string& err,int& errCode)

Honestly, i totally forgot about references lol, I have just been reading up on pointers and it was the first thing to pop to my head.

rozz666
Quote:
 Well, to be honest, you didn't use much of C++ apart from std::cout and std::string. You use pointers where references sould be used and you use macros.

Negative feed back is fine =P just, whats wrong with macros?

##### Share on other sites
Quote:
Original post by Appmonster
Quote:
 You can use the source tags to include code in your post.

I did consider that (its like te only tag I know lol) but I decided it was too large and used pastebin instead.

Don't worry. In case of lots of LOC, scrollbars will automagically appear.

Quote:

Quote:
 Although not apparent here, importing a namespace globally may lead to problems in more complex projects with lots of includes.

I did not know that, the only way i know how to make myself stop having to write std:: before everything is by using that, is there another way I could do it?

Either be less lazy and write the prefix, or in cases where functions or constants in the namespace are heavily used and the using directive seems appropriate, keep it local:
void function(){  using namespace std;  cout << "text" << endl;  cout << "text" << endl;  // lots more ..  cout << "text" << endl;  cout << "text" << endl;}

Quote:
Quote:
 Good candidate for an enumeration:

Yes, good point for the error messages, but for the text colors mabey a different story.
As I understand enumerations kinda assign a Name to a number, starting at one. The text colors are many different numbers (ie 30,14 ect) so im not sure if that would work with the colours.

No, by default it starts at zero an increments by one.

This will start at four and increment by one:
enum ENUM{  ENUM_FIRST = 4,  ENUM_SECOND,  ENUM_THIRD};

This will have 'random' assigned to it:
enum ENUM{  ENUM_FIRST = 4,  ENUM_SECOND = 678,  ENUM_THIRD = 1337};

##### Share on other sites
Quote:
Original post by Appmonster
Quote:
 #define GOODMSG GOOD_MSG

I used that because I was lazy and couldn't be bothered finding the instance of GOODMSG to change it ;p

Search and replace is your friend. ;-) Ctrl+H if you're in Visual Studio.

Quote:
Quote:
 Although not apparent here, importing a namespace globally may lead to problems in more complex projects with lots of includes.

I did not know that, the only way i know how to make myself stop having to write std:: before everything is by using that, is there another way I could do it?

Three options:

1. using namespace std; - stops you having to write std:: before everything, but becomes a pain if it dumps things into scope that conflict with names of things you've already got, meaning you have to clarify which one you want every time.
2. using std::cout; (and so on) - means you can write cout rather than std::cout (or whatever), but you'll have to do this individually for everything you want to do this for.
3. Just write std:: - it's five extra characters, and it usually won't change anything.

EDIT: Or, as WanMaster pointed out, you can restrict its scope as much as possible.

Quote:
Quote:
 Good candidate for an enumeration:

Yes, good point for the error messages, but for the text colors mabey a different story.
As I understand enumerations kinda assign a Name to a number, starting at one. The text colors are many different numbers (ie 30,14 ect) so im not sure if that would work with the colours.

If it's necessary, you can specify the numbers:

enum COLOR{  COLOR_RED = 5,  COLOR_GREEN,  // 6  COLOR_BLUE    // 7 (counts automatically)}or enum COLOR{  COLOR_RED = 5,  COLOR_GREEN = 7,  COLOR_BLUE   // 8 - still counts automatically}or enum COLOR{  COLOR_RED = 14  COLOR_GREEN = 111  COLOR_BLUE = 42}

Quote:
Quote:
 In C++, you are not required to declare all of you're variables at the top of a function. It's a matter of style perhaps, but you might want to declare them where they are first used.

I do like to declare (=P) my variables at the top (grouped by type if you noticed ;P) just because I know where I can find them. If I was to declare them before I use them would it save memory for a little while?

It wouldn't save memory, because they're all allocated on the stack when the function is entered, regardless of where they are declared lexically. It is, however, easier to read because you don't need to scroll back and forth through a function to associate initialization with declaration (I prefer to do both as close together as possible).

Quote:
rozz666
Quote:
 Well, to be honest, you didn't use much of C++ apart from std::cout and std::string. You use pointers where references sould be used and you use macros.

Negative feed back is fine =P just, whats wrong with macros?

They aren't safe - because they're straight text-replacement, they're a) not typesafe (you can't guarantee that a parameter is of a given type), b) they require work to make them behave sanely (i.e. not evaluate parameter multiple times - the classic example being SOME_MACRO(++x) being evaluated) and, as a consequence of both of these c) often appear to work then break without warning when you change some apparently unrelated section of the code.

##### Share on other sites
Rather than point out problems, I thought I'd just lead by example:

#include <iostream>#include <windows.h>#include <string>#include <sstream>using namespace std;// Colour codes higher than this are invalid.// (You should explain why in here; I don't know :) )const int CODE_MAX = 146;enum Color {	GOOD_MSG = 32, // black on green	WHITE    = 7,	ERROR    = 4, // red	OK       = 2, // green	FAT      = 64, // black on red	TITLE    = 14 // yellow};void setTextColor(Color c) {	HANDLE hConsole;	hConsole = GetStdHandle(STD_OUTPUT_HANDLE);	SetConsoleTextAttribute(hConsole, static_cast<int>(c));}void printColoredText(Color c, const string& text) {	setTextColor(c);	cout << text;}void printTitle() {	printColoredText(TITLE, "\n                   ***Windows CMD.exe Color Codes v1.0***\n");	printColoredText(WHITE, "\n                          Author:  Tom Magson \n"	                        "                          Date:   15 July 2008");}// Demonstrate colour codes up to the provided maximum.// Return whether successful. (General note on code design:// it should normally be the calling code's job to interpret// an error; report them as simply as possible.)bool showUpTo(int max) {	if (max >= CODE_MAX || max <= 0) {		return false;	}	for (int i = 1; i <= max; i++) {		cout << "\n[Color Code:" << i << "]";		if (i < 10) {			cout << "  | ";		} else if (i < 100) {			cout << " | ";		} else {			cout << "| ";		}		printColoredText(i, "###");		printColoredText(WHITE, "  |\n----------------|------|");				if (i == 16) {			cout << "\nText with Background:\n";			        "----------------|------|\n";		}	}	return true;}int main() {	printTitle();	bool success = false;	while (!success) {		printColoredText(WHITE, "\n         ");		printColoredText(GOOD_MSG, "!!! 1->15 Text colors. 15->146 Text with Background colors !!!");		printColoredText(WHITE, "\n\nShow colors in range 1 -> ");		// It looks like you were trying to work around the problems that occur		// when the user's input isn't numeric. Here's a robust way to deal with		// user input:		string input;		getline(cin, input);		stringstream parser(input);		int temp;		if (!(parser >> temp)) {			cout << "That's not a number."		} else {			success = showUpTo(temp);			printColoredText(WHITE, "\nReturn: ");			if (success) {				printColoredText(OK, "Complete");			} else {				printColoredText(ERROR, "Selection out of range.\n");			}		}	}	cout << "|" << endl;}

You can probably identify some other peoples' suggestions in here. Sometimes you have to see something to appreciate its value. :)

1. 1
2. 2
Rutin
19
3. 3
4. 4
5. 5

• 14
• 12
• 9
• 12
• 37
• ### Forum Statistics

• Total Topics
631432
• Total Posts
3000041
×