Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » critique code

This thread is locked; no one can reply to it. rss feed Print
 1   2 
critique code
Rick
Member #3,572
June 2003
avatar

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.

1// reading a text file
2#include <iostream.h>
3#include <fstream.h>
4#include <stdlib.h>
5#include <string>
6#include <map>
7 
8//////////////////////////////////////////////////////////////////
9 
10class Leaf
11{
12private:
13public:
14 std::string m_Tag;
15 std::string m_Value;
16 Leaf(std::string tag, std::string value)
17 {
18 m_Tag = tag;
19 m_Value = value;
20 }
21};
22 
23//////////////////////////////////////////////////////////////////
24 
25class Branch
26{
27private:
28public:
29 Branch* m_Prev;
30 std::map<std::string, Leaf*> leafs;
31 std::map<std::string, Branch*> branches;
32 std::string m_Tag;
33 std::string m_Name;
34 void SetPrev(Branch* prev)
35 {
36 m_Prev = prev;
37 }
38 Branch(std::string tag, std::string name)
39 {
40 m_Tag = tag;
41 m_Name = name;
42 }
43 Branch* AddBranch(std::string tag, std::string name)
44 {
45 Branch* temp = new Branch(tag, name);
46 
47 temp->SetPrev(this);
48 branches.insert(std::make_pair(name, temp));
49 //branches.push_back(temp);
50 
51 return temp;
52 }
53 
54 Branch* PreviousBranch()
55 {
56 return m_Prev;
57 }
58 void AddLeaf(std::string tag, std::string value)
59 {
60 Leaf* temp = new Leaf(tag, value);
61 
62 leafs.insert(std::make_pair(tag, temp));
63 //leafs.push_back(temp);
64 }
65};
66 
67//////////////////////////////////////////////////////////////////
68 
69Branch* root;
70Branch* current;
71 
72 
73std::string GetTag(std::string line)
74{
75 std::string::size_type first, second;
76 
77 first = line.find("[",0);
78 second = line.find("]",0);
79 return line.substr(first+1, second - first - 1);
80}
81 
82//////////////////////////////////////////////////////////////////
83 
84std::string GetValue(std::string line)
85{
86 std::string::size_type first, second;
87 
88 first = line.find("\"",0);
89 second = line.find("\"", first+1);
90 
91 //END tags don't have a value
92 if(first != std::string::npos)
93 return line.substr(first+1, second - first - 1);
94 
95 return "";
96}
97 
98//////////////////////////////////////////////////////////////////
99 
100//go through using straight C to parse file
101//loop through buffer
102 
103int main()
104{
105 char buffer[256];
106 std::string tag, value;
107 
108 
109 ifstream file ("C:\\Documents and Settings\\rickp\\Desktop\\test.dlg");
110 
111 while (!file.eof() )
112 {
113 file.getline (buffer,256);
114 
115 //convert to stl string for ease of searching
116 std::string line(buffer);
117 
118 if(line.find("[") != std::string::npos)
119 {
120 //we can get a tag and value here
121 tag = GetTag(line);
122 value = GetValue(line);
123 
124 //we have a leaf
125 if(line.find("=") != std::string::npos)
126 {
127 current->AddLeaf(tag, value);
128 }
129 else
130 {
131 //this is the root tag
132 if(line.find("DIALOG") != std::string::npos)
133 {
134 //create root and set to current
135 root = new Branch(tag, value);
136 current = root;
137
138 }
139 //we are at the end of this branch
140 else if(line.find("END") != std::string::npos)
141 {
142 current = current->PreviousBranch();
143 }
144 //we have a branch that isn't the root
145 else
146 {
147 current = current->AddBranch(tag, value);
148 }
149 }
150 }
151 }
152 
153 int x = atoi(root->branches["cmdExit"]->leafs["TOP"]->m_Value.c_str());
154 
155 return 0;
156}

========================================================
Actually I think I'm a tad ugly, but some women disagree, mostly Asians for some reason.

Rash
Member #2,374
May 2002
avatar

For one thing, use initialization lists wherever you can.

Evert
Member #794
November 2000
avatar

Quote:

#include <iostream.h>
#include <fstream.h>
#include <stdlib.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. (?).
The design was:

1template <class T>
2class bintree {
3 class node {
4 T data;
5 node *daddy, *left, *right;
6 node(node* dad,const T& t) : daddy(dad), left(0), right(0), data(t) {}
7 friend class bintree;
8 public:
9 bool is_leaf() const { return !left && !right; }
10 };
11 node<T> root;
12public:
13 bintree() : bintree(0,0) {}
14 void insert();
15 // more stuff
16};

Rick
Member #3,572
June 2003
avatar

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?

========================================================
Actually I think I'm a tad ugly, but some women disagree, mostly Asians for some reason.

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)
If you don't know that max number of childs.

1template <class T>
2class tree {
3 class node {
4 T data;
5 node *daddy;
6 std::list<node*> childs;
7 node(node* dad,const T& t) : daddy(dad), data(t) {}
8 friend class bintree;
9 public:
10 bool is_leaf() const { return childs.empty(); }
11 };
12 node<T> root;
13public:
14 bintree() : bintree(0,0) {}
15 void insert();
16 // more stuff
17};

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 ;D

--
sig used to be here

Rash
Member #2,374
May 2002
avatar

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
avatar

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]
I like the using references though.

========================================================
Actually I think I'm a tad ugly, but some women disagree, mostly Asians for some reason.

lucaz
Member #4,194
January 2004

I think I read in Bjarne's book, that if you do:
EDIT:

class X {
 ostream& os;
 X(ostring& os_) { os=os_; }  // doesn't works
 X(ostring& os_) : os(os_) {} // works
};

Rash
Member #2,374
May 2002
avatar

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
avatar

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
Gillius's Programming -- https://gillius.org/

Rick
Member #3,572
June 2003
avatar

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]
I see what you mean. In GetTag() I could check the return on the "]" and if std::string::npos return an error

========================================================
Actually I think I'm a tad ugly, but some women disagree, mostly Asians for some reason.

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
- the destructor of Branch should delete the pointers to the Branch and Leaf objects, otherwise you have a memory leak

Rick
Member #3,572
June 2003
avatar

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.

========================================================
Actually I think I'm a tad ugly, but some women disagree, mostly Asians for some reason.

lennaert van der linden
Member #5,320
December 2004

Quote:

Quote:
std::string buffer; file.getline(buffer);

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
avatar

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.

========================================================
Actually I think I'm a tad ugly, but some women disagree, mostly Asians for some reason.

CGamesPlay
Member #2,559
July 2002
avatar

If you use a const string&, you can pass a const char*.

--
Tomasu: Every time you read this: hugging!

Ryan Patterson - <http://cgamesplay.com/>

lucaz
Member #4,194
January 2004

If you don't use reference, you are constructing one more object.
The const, is VERY important, for example:

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.
<code>
int main() {
string s = "hello!";
X MyX(s.c_str()); // ERROR !!!!
};

Rick
Member #3,572
June 2003
avatar

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.

========================================================
Actually I think I'm a tad ugly, but some women disagree, mostly Asians for some reason.

Rash
Member #2,374
May 2002
avatar

You can't supply a const string as an argument to the constructor of Test using your code.

CGamesPlay
Member #2,559
July 2002
avatar

// 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....

--
Tomasu: Every time you read this: hugging!

Ryan Patterson - <http://cgamesplay.com/>

lucaz
Member #4,194
January 2004

Sorry, my last example was wrong.
I'll use your example:

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 


Go to: