|
This thread is locked; no one can reply to it. |
1
2
|
critique code |
Rick
Member #3,572
June 2003
|
OK it's probably sloppy but I'd like some to critique my code. This code reads a file and creates a tree. Kind of like an xml file. I use a tree for naming things. ie.. A branch can have other branches which have leafs etc.. Some obvious things will be making some members public when they shouldn't be, but I was just messing around. I will end up using access functions later.
======================================================== |
Rash
Member #2,374
May 2002
|
For one thing, use initialization lists wherever you can. |
Evert
Member #794
November 2000
|
Quote: #include <iostream.h> I don;t use C++, but it's my understanding that these whould be replaced by #include <iostream> #include <fstream> #include <cstdlib> (I'm not actually sure about the last of these). |
lucaz
Member #4,194
January 2004
|
What do you want to do?. I wrote a binary tree class long time ago. (?).
|
Rick
Member #3,572
June 2003
|
It's not a binary tree though. Each branch can have many branches. I'll change those header files. What do you mean initializer list? ======================================================== |
lucaz
Member #4,194
January 2004
|
I wrote a tree with nodes with N data and N-1 childs. (but it seems this isn't what you want)
|
miran
Member #2,407
June 2002
|
Quote: What do you mean initializer list? This class uses an initializer list: class A { public: int a; int b; A() : a(0), b(5) { // <--- this here is an initializer list } }; EDIT: ha ha -- |
Rash
Member #2,374
May 2002
|
Instead of this: Leaf(std::string tag, std::string value) { m_Tag = tag; m_Value = value; } do this: Leaf(const std::string& tag, const std::string& value) : m_Tag(tag), m_Value(value) { } In other words, treat the data members like you would call constructors of parent classes. I've also changed the function to pass the strings by references. |
Rick
Member #3,572
June 2003
|
Oh OK. I don't like that style though. I see it as it's a function so I'm going to treat it like a function. [EDIT] ======================================================== |
lucaz
Member #4,194
January 2004
|
I think I read in Bjarne's book, that if you do: class X { ostream& os; X(ostring& os_) { os=os_; } // doesn't works X(ostring& os_) : os(os_) {} // works };
|
Rash
Member #2,374
May 2002
|
What are you laughing about? I was hindered by the fact I've noticed he didn't use const references (something you didn't). Initialization lists can sometimes be faster than assignments and are also needed for const data members. |
lucaz
Member #4,194
January 2004
|
Tails, are you talking to me? |
Rash
Member #2,374
May 2002
|
Sorry, no, I'm talking to miran and this dumb oneupmanship. |
gillius
Member #119
April 2000
|
I'm not sure why you wouldn't use the C++ API to parse the file. Regardless, besides the other suggestions I agree with (const reference, wrong includes, initializer lists), you need to do more error handling. I am referring specifically to at least the GetTag and GetValue functions. You do check for the start of a tag in your main loop but you never check for the existence of the end one. This would be a good example of a time to use exceptions. Gillius |
Rick
Member #3,572
June 2003
|
Quote: I'm not sure why you wouldn't use the C++ API to parse the file. I've never done it before. Could you point me in the right direction? Quote: You do check for the start of a tag in your main loop but you never check for the existence of the end one. [END] is the end of any tag. The file looks like this. [DIALOG] "dlgTest" [TOP]="10" [BUTTON] "cmdExit" [TOP]="15" [END] [END]
[EDIT] ======================================================== |
lennaert van der linden
Member #5,320
December 2004
|
Hi, The code looks OK (I haven't tested it), but I think you should use XML instead. You get the same results and you don't have to write (and maintain) the parsing functions. Of course you then don't get to enjoy writing your own parser and tree container Apart from the initialization list (which is a good thing) and the use of references I have 3 minor "critiques": - You could read a string instead of a 256 character buffer, unless you specifically want to limit this for some reason: e.g. std::ifstream file ("C:\\Documents and Settings\\rickp\\Desktop\\test.dlg"); while (!file.eof() ) { std::string buffer; file.getline(buffer); ... }
- the root and current variables should be in main() function scope, not outside |
Rick
Member #3,572
June 2003
|
Quote: the root and current variables should be in main() function scope, not outside These will be in another class when I'm finished. Quote: std::string buffer; file.getline(buffer); This doesn't work for me. ======================================================== |
lennaert van der linden
Member #5,320
December 2004
|
Quote:
Quote: This doesn't work for me. Sorry, my mistake. It should be: std::string buffer; std::getline( file, buffer );
|
Rick
Member #3,572
June 2003
|
That works, thanks. Also why do I want to use const references in my constructors? Is this so it doesn't create another string when it's passed in(the reference part)? The const doesn't seem like something I need. I mean I'm the only one going to use this and it doesn't change these values. ======================================================== |
CGamesPlay
Member #2,559
July 2002
|
If you use a const string&, you can pass a const char*. -- Ryan Patterson - <http://cgamesplay.com/> |
lucaz
Member #4,194
January 2004
|
If you don't use reference, you are constructing one more object. class X { string s; X(string s_) { s=s_; } // bad, initializing s two times, creating an extra temporal string X(string s_) : s(s_) {} // bad, creating an extra object temporal string X(string& s_) : s(s_) {} // <!!!> this isn't bad a first sight, but... }; int main() { const string path = "/home/local"; X MyX(path); // ERROR, can't convert const string& to string&. } The are a lot of examples, use const whenever you can. I can't remember well, but I think that allegro 403 function mask_color(BITMAP*) wasn't a const BITMAP*, and I need to redesign a lot of things. (I'm not sure, but I think that that function don't need to modify the bitmap) EDIT: Quote: I mean I'm the only one going to use this and it doesn't change these values. Yes, that's true, but you use other people libs, like the stl and allegro. This is another example using the previous class X. |
Rick
Member #3,572
June 2003
|
So I could do this: class Test { string data; public: Test(string& a) : data(a) {} }; int main() { string hello = "What the..."; Test hi(hello); return 0; }; Right now I don't see a reason to use the const part. I actually might want some data to change. Thanks for the examples though. ======================================================== |
Rash
Member #2,374
May 2002
|
You can't supply a const string as an argument to the constructor of Test using your code. |
CGamesPlay
Member #2,559
July 2002
|
// I didn't want to use const because I might at some point want to change that data passed to the constructor of this class cls::cls(/*const*/ string& s): m_s(s) { } Riiiiight.... -- Ryan Patterson - <http://cgamesplay.com/> |
lucaz
Member #4,194
January 2004
|
Sorry, my last example was wrong. class Test { string data; public: Test(char* a) : data(a) {} // get a char*, instead of a string, is a better choice, because not all people use strings }; int main() { string hello = "What the..."; Test hi(hello.c_str()); // ERROR! return 0; };
|
|
1
2
|