2
\$\begingroup\$

I've got a rather long function that will take the size of a triangle print it with stars and give the user the option of whether or not they want stars printed inside the triangle, and the orientation (point up or down). I was hoping for some general refactoring methodologies I could apply.

Every permutation is possible for inverted/filled, inverted/not filled, etc. What are some good general methods I can apply here to get some more code reuse?

void Triangle::Print(string direction, string fill)
{
    string starWithSpace = "* ";

    for (int i = leg1 - 1, j = 1; i >= 0; i--, j++, j < i)
    {
        if (direction == "vertical")
        {
            cout << string(i, ' ');

            if (fill == "yes")
            {
                int repeat = 0;
                while (repeat < j)
                {
                    cout<< starWithSpace;
                    repeat++;
                }
            }
            else if (fill == "no")
            {
                if (j * 2 - 3 < 0)
                {
                    cout << "*";
                }

                if ( j > 1 && i >= 1)
                {
                    cout << "*";
                    cout << string(((j * 2 - 3) < 0) ? 1 : j * 2 - 3, ' ');
                    cout << "*";

                }
                if (i == 0)
                {
                    int repeat = 0;
                    while (repeat < leg1)
                    {
                        cout << starWithSpace;
                        repeat++;
                    }
                }


            }
            cout << "\n";
        }
        else if (direction == "inverted")
        {
            //print bottom line
            if (j == 1)
            {
                int repeat = 0;
                while (repeat < leg1)
                {
                    cout << starWithSpace;
                    repeat++;
                }
            }
            cout << string(j - 1, ' ');
            if (j != 1 && fill == "yes")
            {
                int repeat = 0;
                while (j >1 && repeat < i +1)
                {
                    cout << starWithSpace;
                    repeat++;
                }
            }
            else if (fill == "no" && j!= 1)
            {
                int skipFactor = i * 2 - 1;
                cout << "*";
                cout << string(skipFactor > 0 ? skipFactor: 1, ' ');
                if (i != 0)
                {
                    cout << "*";
                }
            }
            cout << "\n";
        }
    }
    cout << "\n";
}
\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$

These comments apply to your question, not your answer.

Using strings as arguments seem unnecessary, it would be better to use bools or enums, that way if you one day change strings (e.g. another language) it does not affect the print function.

void Triangle::Print(Direction direction, bool fill)

There are magic numbers in your code 2,3 etc. try using constants for those values with descriptive names.

The function is arguably a bit long and has quite a few if-statement levels, better to split into several functions that you can parameterize.

You may also want to present public functions to the user that do not have all the arguments then leave the original Print protected e.g.

PrintVertical(bool fill)
PrintInverted(bool fill);
...

that may make its use less confusing than keeping track of all possible argument combinations.

You may also want to include the stream to print to as an argument to Print, that way if you can alternatively print to a file instead of cout.

\$\endgroup\$
0
\$\begingroup\$

Ok, so this took WAY WAY longer than I expected, but here's my go at a refactor:

void Triangle::Print(string fill, string direction)
{
    int n = 1;
    while (n < leg1)
    {
        this->PrintLine(fill, direction, n);
        n++;
    }
    if (direction == "vertical")
    {
        this->PrintBottom(direction);
    }

    cout << "\n";
}

void Triangle::PrintLine(string fill, string direction, int currentLine)
{
    bool verticalPattern = (currentLine * 2) - 3 > 0;

    if (fill == "empty")
    {
        if (direction == "vertical")
        {

            cout << string(leg1 - currentLine, ' ');
            int fillExpression = (currentLine == 2) ? 1 : currentLine * 2 - 3;
            cout << "*";
            cout << string(abs(currentLine * 2 - 3), ' ');
            if (currentLine != 1)
            {
                cout << "*";
            }

        }
        else if (direction == "inverted")
        {
            if (currentLine == 1)
            {
                int n = 0;
                while (n < leg1)
                {
                    cout << "* ";
                    n++;
                }
            }
            else
            {
                currentLine--;
                int fillExpression = abs((leg1 - currentLine) * 2 - 3);
                cout << string(currentLine, ' ');
                cout << "*";
                cout << string(fillExpression, ' ');
                cout << "*";
                if (leg1 - 2 ==  currentLine)
                {
                    cout << "\n";
                    cout << string(currentLine+1, ' ');
                    cout << "*";
                }
            }
        }
    }
    else if (fill == "filled")
    {
        if (direction == "vertical")
        {
            int row = 0;

            cout << string(leg1 - currentLine , ' ');

            while (row < currentLine )
            {
                cout << "* ";
                row++;
            }

        }
        else if (direction == "inverted")
        {
            cout << string(currentLine -1, ' ');

            if (leg1 - currentLine != 0 || leg1 - currentLine == 1)
            {
                int n = 0;
                while (n <= (leg1 - currentLine) )
                {
                    cout << "* ";
                    n++;
                }
            }
            if(currentLine == leg1-1)
            {
                cout << "\n";

                cout << string(currentLine, ' ');
                cout << "*";
            }

        }
    }

    cout << "\n";

}
void Triangle::PrintBottom(string direction)
{
    bool b = direction == "vertical";
    if (b)
    {
        int n = leg1;
        while (n > 0)
        {
            cout << "* ";
            n--;
        }
    }
    else
    {
        cout << string(leg1 - 1, ' ') << "*";
    }
}

So what this does is print a triangle inverted or vertical and have the inside either filled with stars or empty.

* * * * * 
 *    *
  *  *
   *

this is an example output of empty and inverted. The problem I ran into with the PrintLine function is that I wanted the entire bottom row to contain stars, and the inner rows to not. This lead to some problems calling my PrintLine function with the fill set to empty and I had to jury rig it do work a bit.

\$\endgroup\$

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.