Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

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.

Source Link
user1118321
  • 11.9k
  • 1
  • 20
  • 46

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.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /