2
\$\begingroup\$

If one enters, say, "2, j" then it prints:

j
jj

If there is no second it should be *, so if it's just 2, it's * **.

#include <iostream>
#include <string>
#include <vector>

using std::string; using std::cin; using std::cout; using std::endl; using std::ostream; using std::vector;

class tree {
public:
    friend ostream &output(tree &oTree, ostream &os);
    bool makeTree(int, const string&);
private:
    vector<string> Tree;
};

bool tree::makeTree(int newBase, const string &newCont = "*") {
    if (!Tree.empty())
        Tree.clear();
    string tempString;
    for(int tempbase = 0; tempbase != newBase; ++tempbase) {
        tempString += newCont;
        Tree.push_back(tempString);
    }
  return 1;
}

ostream &output(tree &oTree, ostream &os) {
    for (auto c: oTree.Tree) {
        os << c << endl;
    }
    return os;
}


int main() {

}
\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

Don't do this (its not readable).

using std::string; using std::cin; using std::cout; using std::endl; using std::ostream; using std::vector;

One declaration per line.

using std::string;
using std::cin;
using std::cout;
using std::endl;
using std::ostream;
using std::vector;

But actually prefer not to do it at all. If you must do it then confine it to the most narrow scope possible. The whole reason that the standard library is std:: is so that it does not cost much to prefix members by it.

std::cout << "Hello World" << std::endl; // Not that hard

It cost nothing to clear an empty tree.

if (!Tree.empty())  // Why do this test. Just call clear.
    Tree.clear();

In C++ the & and * are usually placed with the type (its type information). This is different from C.

bool tree::makeTree(int newBase, const string &newCont = "*") {
                                             ^^^  Looks strange.


// I would write it like this:
// Note: I also put the const the other side (but that's more a personal 
// preference and there is no clear better style there).
bool tree::makeTree(int newBase, string const& newCont = "*") {

Don't use integers to represent bools. At the end of makeTree() I see.

return 1;

// Use a boolean value.
return true;

But there is no failure value so why return anything at all?

Prefer '\n' over std::endl. The difference between the two is that std::endl does an extra flush. This can make using it very inefficient. The whole point of using the buffer is to make printing efficient and the streams are flushed at optimal points automatically and forcing a flush will make the use of stream less optimal.

\$\endgroup\$
1
  • \$\begingroup\$ Hey I took the suggestion to make the makeTree function output also but I didnt really like it all there so you mind telling me which one you like better? new but original pastebin.com/aXUNcrq8 and new with output in func pastebin.com/UBY2UtJf \$\endgroup\$ Commented Jan 13, 2016 at 19:41
1
\$\begingroup\$

I don't see where you gain anything by generating the entire tree in memory, then writing it out. Lacking a reason to do otherwise, I'd print out each line as it's generated.

Considering that you're using std::string anyway, I'd look carefully at its constructors to see if I could use the one that lets you specify a character and a length to simplify the code a little bit.

I'd avoid using std::endl...ever, probably. On the rare occasion that you really need what it does (and it really is rare) I think it's better to do something like std::cout << "\n" << std::flush; instead (to make it clear that you really do want what it does).

\$\endgroup\$
2
  • \$\begingroup\$ I'm going to fix the endl thing and I was thinking of making the code generate the tree on the output since there would be no difference unless I was to use the code were I want the tree but dont want to output it so it might make it less convinent? can someone tell me? \$\endgroup\$ Commented Jan 13, 2016 at 15:38
  • \$\begingroup\$ @Magirldooger: If there were honestly a use for a tree as a string, producing it that way might be useful--but in this case, you're just asked to print it out, and I've yet to see a real use for this kind of output other than just displaying it. Similar projects are common as homework to practice how loops work and such, but don't seem to be of great value otherwise. \$\endgroup\$ Commented Jan 13, 2016 at 18:49
-1
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ friend does not break data hiding it improves encapsulation and in this case is the perfect example of good usage. programmers.stackexchange.com/a/99595/12917 usign your technique to expose Tree you are definately breaking encapsulation because you exposing the implementation detail of Tree thus you are breaking the open/close principle. \$\endgroup\$ Commented Jan 13, 2016 at 14:12
  • \$\begingroup\$ @LokiAstari I read in my book input/output functions should not be member functions and you'r example shows that too but why is there a issue with it? \$\endgroup\$ Commented Jan 13, 2016 at 15:43
  • \$\begingroup\$ @Magirldooger: In my opinion your implementation has no problems to that affect. user1118321 is claiming it should not be a friend. I disagree with him. \$\endgroup\$ Commented Jan 13, 2016 at 19:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.