Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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());
        }
    }
}
share|improve this question
up vote 6 down vote accepted

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.

share|improve this answer
    
30 calls to a trivial subtraction, multiplication, and addition worries you? What was it they say about "premature optimization"? – jpmc26 Mar 12 at 0:44
  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.
share|improve this answer
// 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.

share|improve this answer

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.

share|improve this answer
1  
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). – Luis Masuelli Mar 11 at 16:58
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.

share|improve this answer
1  
Shouldn't it be (height -1) + 1/2 after simplifying? – JRLambert Mar 11 at 21:40
    
@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. – Taemyr Mar 12 at 14:54
    
Fair point. Was looking at it purely from a mathematical standpoint. – JRLambert Mar 12 at 14:58

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.

share|improve this answer

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 http://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)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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