In addition to the other answers, I see some things that could be improved. Here are a few suggestions:
Naming
#Naming
It'sIt's confusing to name the class tree
and name a member variable Tree
. The member variable represents the "branches" of the tree so maybe a better name would be branches
?
Speaking of naming, you should add names to the prototype of the makeTree()
method. It will be confusing to users who want to call the method what the inputs are if there are no names.
In the loop where the strings are created you have tempString
and tempbase
. While these are accurate names, they are not very good names. The string held by tempString
is going to be the next "branch" in the tree, so you could name it branch
or nextBranch
. It's not clear what "base" means in either newbase
or tempbase
. I would change all instances of base
to length
. I'd rename newbase
to maxLength
and tempbase
to branchLength
.
Use of friend
#Use of friend
II dislike the use of friend
in this case. I'd prefer that there was an accessor to retrieve a const
reference to oTree.Tree
, and then you could just write a normal stream insertion operator function. friend
should be used sparingly as it breaks the rule of data hiding.
Construction
#Construction
YouYou have a makeTree()
method, but no constructor. It seems like makeTree()
should just be a constructor for the class (and hence should be called tree()
). As it stands, a caller could make an instance of tree
and it would be invalid (or at least empty). If the only public constructor was one which required them to pass in the depth of the tree and the character, they'll never create an empty tree.
In addition to the other answers, I see some things that could be improved. Here are a few suggestions:
#Naming
It's confusing to name the class tree
and name a member variable Tree
. The member variable represents the "branches" of the tree so maybe a better name would be branches
?
Speaking of naming, you should add names to the prototype of the makeTree()
method. It will be confusing to users who want to call the method what the inputs are if there are no names.
In the loop where the strings are created you have tempString
and tempbase
. While these are accurate names, they are not very good names. The string held by tempString
is going to be the next "branch" in the tree, so you could name it branch
or nextBranch
. It's not clear what "base" means in either newbase
or tempbase
. I would change all instances of base
to length
. I'd rename newbase
to maxLength
and tempbase
to branchLength
.
#Use of friend
I dislike the use of friend
in this case. I'd prefer that there was an accessor to retrieve a const
reference to oTree.Tree
, and then you could just write a normal stream insertion operator function. friend
should be used sparingly as it breaks the rule of data hiding.
#Construction
You have a makeTree()
method, but no constructor. It seems like makeTree()
should just be a constructor for the class (and hence should be called tree()
). As it stands, a caller could make an instance of tree
and it would be invalid (or at least empty). If the only public constructor was one which required them to pass in the depth of the tree and the character, they'll never create an empty tree.
In addition to the other answers, I see some things that could be improved. Here are a few suggestions:
Naming
It's confusing to name the class tree
and name a member variable Tree
. The member variable represents the "branches" of the tree so maybe a better name would be branches
?
Speaking of naming, you should add names to the prototype of the makeTree()
method. It will be confusing to users who want to call the method what the inputs are if there are no names.
In the loop where the strings are created you have tempString
and tempbase
. While these are accurate names, they are not very good names. The string held by tempString
is going to be the next "branch" in the tree, so you could name it branch
or nextBranch
. It's not clear what "base" means in either newbase
or tempbase
. I would change all instances of base
to length
. I'd rename newbase
to maxLength
and tempbase
to branchLength
.
Use of friend
I dislike the use of friend
in this case. I'd prefer that there was an accessor to retrieve a const
reference to oTree.Tree
, and then you could just write a normal stream insertion operator function. friend
should be used sparingly as it breaks the rule of data hiding.
Construction
You have a makeTree()
method, but no constructor. It seems like makeTree()
should just be a constructor for the class (and hence should be called tree()
). As it stands, a caller could make an instance of tree
and it would be invalid (or at least empty). If the only public constructor was one which required them to pass in the depth of the tree and the character, they'll never create an empty tree.
In addition to the other answers, I see some things that could be improved. Here are a few suggestions:
#Naming
It's confusing to name the class tree
and name a member variable Tree
. The member variable represents the "branches" of the tree so maybe a better name would be branches
?
Speaking of naming, you should add names to the prototype of the makeTree()
method. It will be confusing to users who want to call the method what the inputs are if there are no names.
In the loop where the strings are created you have tempString
and tempbase
. While these are accurate names, they are not very good names. The string held by tempString
is going to be the next "branch" in the tree, so you could name it branch
or nextBranch
. It's not clear what "base" means in either newbase
or tempbase
. I would change all instances of base
to length
. I'd rename newbase
to maxLength
and tempbase
to branchLength
.
#Use of friend
I dislike the use of friend
in this case. I'd prefer that there was an accessor to retrieve a const
reference to oTree.Tree
, and then you could just write a normal stream insertion operator function. friend
should be used sparingly as it breaks the rule of data hiding.
#Construction
You have a makeTree()
method, but no constructor. It seems like makeTree()
should just be a constructor for the class (and hence should be called tree()
). As it stands, a caller could make an instance of tree
and it would be invalid (or at least empty). If the only public constructor was one which required them to pass in the depth of the tree and the character, they'll never create an empty tree.