2
\$\begingroup\$

I have created a simple terminal program: it allows the user to enter a height and it will produce triangles of asterisks (*'s) based on the height.

I have just started learning C# as practice for my internship. I would appreciate any feedback.

using System;

public class Triangle
{
    public int height;
    private string pyramid = "";

    public Triangle(int height)
    {
        this.height = height;
    }

    public String makeTriangle()
    {
        //4 
        //   *    = 1         ***+***  = Even on both sides  7/2  
        //  ***   = 3 - (+2)  **+++**  = 
        // *****  = 5 - (+2)  *+++++*
        //******* = 7 - (+2)  +++++++
        int cutEnds = biggestOddNumber() / 2;

        for (int i = 0; i < height; i++)
        {
            for (int j = 1; j <= biggestOddNumber(); j++)
            {
                String singleElement = "*";

                if (j <= cutEnds || j > (biggestOddNumber()-cutEnds))
                {
                    pyramid = pyramid + " ";
                }
                else
                {
                    pyramid = pyramid + singleElement;
                }

                if (j == biggestOddNumber())
                {
                    pyramid = pyramid + "\r\n";
                }
            }
            cutEnds--;

        }
        return pyramid;
    }

    public int biggestOddNumber()
    {
        // 1 2 3 4 5 6 7 8 9 10
        // ?   ?   ?   ?
        //                  
        //Formula for Odd Numbers = 2n + 1
        //2*0 + 1 = 1
        //2*1 + 1 = 3
        //2*2 + 1 = 5
        //...
        // we do height -1 since if you look at formula we start from 0 not 1 
        // if we do 2*4 + 1 = (9) 
        //4 
        //   *    = 1        
        //  ***   = 3 - (+2)
        // *****  = 5 - (+2)
        //******* = 7 - (+2) However it should be (7)

        return (2 * (height-1) + 1);

    }
}

Main method

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {

            Console.WriteLine("Enter height of pyramid");
            int height = int.Parse(Console.ReadLine());
            Triangle triangle = new Triangle(height);
            Console.WriteLine(triangle.makeTriangle());
        }
    }
}
\$\endgroup\$
6
\$\begingroup\$

public String makeTriangle(): In general it is recommended to use the types specific to the language, so string instead of String, int instead of Int32, etc.


public String makeTriangle(), public int biggestOddNumber(): methods need to be PascalCase.


public int height;: is this supposed to be a field or a property? If it is a field it should be private, if it's a property it should be PascalCase and should have a getter/setter.


public int biggestOddNumber(): does this method need to be public? Does this even need to be a method? I'd be inclined to make it a property:

public int BiggestOddNumber
{
    get
    {
        // 1 2 3 4 5 6 7 8 9 10
        // ?   ?   ?   ?
        //                  
        //Formula for Odd Numbers = 2n + 1
        //2*0 + 1 = 1
        //2*1 + 1 = 3
        //2*2 + 1 = 5
        //...
        // we do height -1 since if you look at formula we start from 0 not 1 
        // if we do 2*4 + 1 = (9) 
        //4 
        //   *    = 1        
        //  ***   = 3 - (+2)
        // *****  = 5 - (+2)
        //******* = 7 - (+2) However it should be (7)

        return (2 * (height-1) + 1);
    }
}

If you want to keep it a method, you'll need to change its name since a method name should contain a verb.

More worrying to me is that you repeatedly call this, so it is repeatedly calculated, but this is pointless since height won't ever change. Right now if height is 10, biggestOddNumber() gets called over thirty times!

If you don't need to expose this biggestOddNumber it could simply be a private field which you set in the constructor public Triangle(int height). Even if you need to expose it, I'd still advice you to set a private field in the constructor and have a get-only property to expose it.


Don't use "\r\n", use Environment.NewLine.


You seem confident your users won't enter an invalid value, e.g. "-" or "boo". Don't trust input, always check!

Console.WriteLine("Enter height of pyramid");
int height = int.Parse(Console.ReadLine());

There is no need to assign triangle, just do it in one line:

Console.WriteLine(new Triangle(height).makeTriangle());

To end on a positive note: I'm happy to see that the Main() in Program is very limited and that you made a separate Triangle class to hold all of the logic.

\$\endgroup\$
1
  • \$\begingroup\$ 30 calls to a trivial subtraction, multiplication, and addition worries you? What was it they say about "premature optimization"? \$\endgroup\$
    – jpmc26
    Mar 12 '16 at 0:44
6
\$\begingroup\$
  1. You should read Naming guidelines and apply them to your code.
  2. pyramid should be a local variable. Otherwise calling makeTriangle more than once will produce incorrect results.
  3. There is no need for biggestOddNumber to be public.
  4. singleElement should be a constant field.
  5. You can use String.PadRight and String.PadLeft methods to pad the string with spaces (or any other character) from both sides. Instead of manually doing it in hard-to-understand loop.
\$\endgroup\$
5
\$\begingroup\$
// we do height -1 since if you look at formula we start from 0 not 1 

I think you've done a great job of creating comments that say why, not what. That's very promising. Well done.

You also did a great job of keeping your main method small and the "business" logic separated from the input/output of the program.

One small thing to mention in addition to what's already been said.

    int cutEnds = biggestOddNumber() / 2;

    for (int i = 0; i < height; i++)
    {
        for (int j = 1; j <= biggestOddNumber(); j++)
        {
            //...
        }
        cutsEnd--;
    }

Can be rewritten as

    int cutEnds = biggestOddNumber() / 2;

    for (int i = 0; i < height; i++, cutsEnd--)
    {
        for (int j = 1; j <= biggestOddNumber(); j++)
        {
            //...
        }
    }

Which is a bit nicer because all of our increments/decrements happen in one place.

\$\endgroup\$
4
\$\begingroup\$

Don't repeat class name in function name

triangle.makeTriangle() does not look good. Simply make will be better.

Don't use verbs in variable names (unless it is a reason)

cutEnds sounds like a function or a boolean variable (some flag). Think what this variable really stores.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Would like to add on names and verbs: imperative verbs are for functions which act like procedures in their semantics, while declarative verbs are for boolean variables or boolean functions (Other types should be nouns. Adjectives by their own, when used as names, act like "isADJECTIVE", belonging to booleans). \$\endgroup\$ Mar 11 '16 at 16:58
3
\$\begingroup\$
cutEnds = biggestOddNumber() / 2;

inlining

cutEnds = (2 * (height-1) + 1)/2;

distributing(*)

cutEnds = (2*(height-1)/2)+1/2;

simplifying(*)

cutEnds = height-1;

(*) These steps are only valid because we know that 2*(height-1) is even.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Shouldn't it be (height -1) + 1/2 after simplifying? \$\endgroup\$
    – JRLambert
    Mar 11 '16 at 21:40
  • \$\begingroup\$ @JRLambert Not really. We know that (2*(height-1)/2+1) is an odd number. We divide it by two, and according to language specification that division rounds down. \$\endgroup\$
    – Taemyr
    Mar 12 '16 at 14:54
  • \$\begingroup\$ Fair point. Was looking at it purely from a mathematical standpoint. \$\endgroup\$
    – JRLambert
    Mar 12 '16 at 14:58
2
\$\begingroup\$

As you are not limiting the input, consider using StringBuilder instead of string concatenation. It's more efficient.

If you don't know at compile time how many times concatenation will be executed, it's a rule of thumb to use StringBuilder.

\$\endgroup\$
1
\$\begingroup\$

This loop looks a bit funny to me:

    int cutEnds = biggestOddNumber() / 2;

    for (int i = 0; i < height; i++)
    {
        for (int j = 1; j <= biggestOddNumber(); j++)
        {
            String singleElement = "*";

            if (j <= cutEnds || j > (biggestOddNumber()-cutEnds))
            {
                pyramid = pyramid + " ";
            }
            else
            {
                pyramid = pyramid + singleElement;
            }

            if (j == biggestOddNumber())
            {
                pyramid = pyramid + "\r\n";
            }
        }
        cutEnds--;

    }

Importantly, the last check: if (j == biggestOddNumber()) means that this operation can (and should) be after the if.

    int cutEnds = biggestOddNumber() / 2;

    for (int i = 0; i < height; i++)
    {
        for (int j = 1; j <= biggestOddNumber(); j++)
        {
            String singleElement = "*";

            if (j <= cutEnds || j > (biggestOddNumber()-cutEnds))
            {
                pyramid = pyramid + " ";
            }
            else
            {
                pyramid = pyramid + singleElement;
            }
        }
        pyramid = pyramid + "\r\n";
        cutEnds--;
    }

As somebody rightfully pointed out, the cutEnds-- can be inside the same loop definition as int i.

    int cutEnds = biggestOddNumber() / 2;

    for (int i = 0; i < height; i++, cutEnds--)
    {
        for (int j = 1; j <= biggestOddNumber(); j++)
        {
            String singleElement = "*";

            if (j <= cutEnds || j > (biggestOddNumber()-cutEnds))
            {
                pyramid = pyramid + " ";
            }
            else
            {
                pyramid = pyramid + singleElement;
            }
        }
        pyramid = pyramid + "\r\n";
    }

Now, one funny thing which may take some time to realise, but cutEnds == height - 1. This gives us a possibility to remove i.

    int cutEnds = biggestOddNumber() / 2;

    for (int cutEnds = biggestOddNumber() / 2; cutEnds >= 0; cutEnds--)
    {
        for (int j = 1; j <= biggestOddNumber(); j++)
        {
            String singleElement = "*";

            if (j <= cutEnds || j > (biggestOddNumber()-cutEnds))
            {
                pyramid = pyramid + " ";
            }
            else
            {
                pyramid = pyramid + singleElement;
            }
        }
        pyramid = pyramid + "\r\n";
    }

Inspection of the route the code takes, sees you go cutEnds times into the first branch of the if, biggestOddNumber() - 2 * cutEnds, the second branch, and cutEnds into the first branch again. Let's see what we can do about that. (I just googled 'C# multiply string, and found https://stackoverflow.com/a/533204, which is relevant).

    for (int cutEnds = height - 1; cutEnds >= 0; cutEnds--)
    {
        pyramid = pyramid + new string(' ', cutEnds);
        pyramid = pyramid + new string('*', biggestOddNumber() - 2 * cutEnds);
        pyramid = pyramid + new string(' ', cutEnds);
        pyramid = pyramid + "\r\n";
    }

(Some languages also allow an easy method to center strings, which would be useful, but C# apparently does not (from quick google search).

(Disclaimer: I did not test this)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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