Program that prints an ASCII pyramid

Question

I have written the following program in C# to display an ASCII pyramid.

public static void method1()
{
  int k;
  int kCond = 5;
  int i;
  int inc = 0;
  for (int p = 0; p <=5; p++)
  {
    for(k = 0; k <= kCond; k++)
    {
         Console.Write(" ");
    }
     kCond--;
     for(i=0; i<=inc; i++)
     {
        Console.Write("*")
     }
     inc +=2;
     Console.WriteLine();
  }
}

Output:

         *
        ***
       *****
      *******
     *********
    ***********

My question is, is this good code from a performance point of view, or not? How could I improve it?

Answer 1

I took your code and started condensing it, and came up with the following improvements that could be made:

• Change the inner loops to use the string constructor that takes a character and a count instead of using a loop
• Give the method and variables more meaningful names
• Add a size parameter that can be passed to the method
• Put declarations and incrementing of variables in the for statement
• Use math operations on the counter rather than tracking multiple variables that increment at constant rates relative to the counter
• Use Enumerable.Range as a looping mechanism to select the strings along with string.Concat to join them together
• Have the method return a string so it can be written by the client to other things (like a log file or another control)

Code:

public static string GetPyramid(int size)
{
    return string.Concat(Enumerable.Range(0, size).Select(count =>
        new string(' ', size - count) + 
        new string('*', count * 2 + 1) + 
        Environment.NewLine));
}

Answer 2

Rather than send to the console as you build. Build a string then send to the console. This avoids some of the overhead associated with writing to IO.

In terms of performance you can use a System.Text.StringBuilder and avoid some of the allocation overhead of new String. StringBuilder will allocate new memory as the string grows, doubling the buffer each time it does so.

StringBuilder.Append can be used to add a number of char at a time reducing the code complexity.

You can write the built string directly to the console, or if needed convert it to a string via StringBuilder.ToString.

As a function, passing the pyramid size would make it more useful. As an example you can also use an optional argument to define what the pyramid is built of. Optional arguments require a default value

public static void ConsolePyramid(int size, char block = '#') {
    StringBuilder pyramid = new StringBuilder();
    for (int i = 0; i <= size; i++) {
        pyramid.Append(' ', size - i);
        pyramid.Append(block, i * 2 + 1);
        pyramid.Append(Environment.NewLine);
    }       
    Console.Write(pyramid);
}

Or

public static string BuildPyramidString(int size, char block = '#') {
    StringBuilder pyramid = new StringBuilder();
    for (int i = 0; i <= size; i++) {
        pyramid.Append(' ', size - i);
        pyramid.Append(block, i * 2 + 1);
        pyramid.Append(Environment.NewLine);
    }       
    return pyramid.ToString();
}

Answer 3

Performance

Why do you care about performance, and what aspect of performance matters to you? Since this method takes no inputs, you can probably get better performance by hard-coding the output string, but...

... the performance will be fine: you haven't fallen into the usual trap of concatenating strings. It might be faster to use a StringBuilder to assemble the whole thing and print it out in one, or to generate whole lines at a time, but this is exactly the sort of thing you shouldn't be worrying about unless you have evidence that it is a real problem; reusability and maintainability are significantly more important concerns (so you don't ever have to rewrite the same functionality).

Other

Rufus L presented a nice method which returns a String instead of printing directly to the console. Another approach would be to pass PrintPyramid a TextWriter, so that you can have it write to wherever you want. I would rename the method to WriterPyramid, since that is the usual theme in .NET. Throw in some inline documentation (///) to describe the behaviour, and we have a sensible API:

/// <summary>
/// Writes a pyramid of stars the given height to the given TextWriter
/// </summary>
public static void WritePyramid(int height, TextWriter writer)
{
    // TODO: implement
}

You can then pass it System.Console.Out to print to the console, but you can pass it something else if you need to (such as a StringWriter), e.g. when you are testing it.

For something so simple this probably isn't worth it, but if you were writing lots of text, or would be writing it out in a later method call, then passing TextWriters around can be very handy.

---

It is generally advisable to define variables as close to their usage as possible, as it makes their purpose more apparent, and reduces the opportunity for misusing them. For example, as Rufus L suggests, don't forward-declare i and k. They are just counters, and shouldn't be available outside of their respective loops.

---

Be consistent with your white-space. You have p <=5, k <= kCond, and i<=inc all in one method. Generally I go with whatever the IDE of choice does, and in C# that usually means i <= inc.

Answer 4

Indentation & spacing: This may not seem important, but it's a very easy and useful habit to get into. For example, it should be obvious at a glance how many for loops a given line of code is inside. With this code as-is, two different Console.Write commands that actually execute the same number of times are at different indentation levels.

There are also inconsistencies with spacing around operators. For example, we have p <=5, i<=inc, and k <= kCond. Personally I prefer the last.

---

Increment & Decrement operators: This may not seem important, but I believe the i++ and kCond-- should be replaced by explicit i += 1 and kCond -= 1. Some modern languages don't even have them. Even the technical reviewer of the C# specification regrets their inclusion (scroll to #3). Avoiding the use of these operators can prevent confusing scenarios.

---

Minimize Scope: This may not seem important, but the variable k is never referenced outside of its for loop. Declare it there, as you did for p. The same goes for i. Restricting variable references to the minimum possible scope makes the code easier to read. As it stands, I have to hold the variable "k" in my head as I read through the function. If it only existed inside its own loop, I could to safely forget about it.

This may feel inefficient - if you are declaring i many times, are you not allocating memory for it multiple times? The answer is no. The C# compiler is smarter than that, so the performance is exactly the same.

---

Reduce Repetition: This may not seem important, but right now you have two for loops that are almost identical: One for printing " ", and one for printing "*". If you create a function

private static void PrintMultiple(string str, int times)

which calls Console.Write in a little for loop, you move that repetition out of your main function, making it easier to read. I'll leave the method itself to you.

This may feel inefficient - if you are calling a method inside of a loop, are you not incurring method call overhead many times? The answer is maybe. The C# compiler is smart enough, in some cases, to inline method calls. The only way to determine if it's doing that for your code would be to inspect the IL code generated after introducing this method, or perhaps doing some very precise speed tests.

For a sense of scale, however, this developer noticed that a method call was not being inlined, and the performance impact on their code was noticeable: a 0.4 second slowdown for billions of method calls. When printing a 6-line ASCII pyramid, you are unlikely to notice a difference.

If, however, you are very concerned by nanosecond delays, one approach you might consider is building your strings in larger chunks using the new String(char, int) constructor. You may find it to be slightly more efficient than printing them one character at a time. Then again, you may not. Only extensive testing or IL inspection will tell.

---

Direct Formulas: This may not seem important, but it's easier to reason about variable assignments than variable modifications. Since the value of kCond changes by a constant amount when (and only when) the value of p changes, you can calculate it directly instead of repeatedly subtracting from its initial value.

This also allows you to declare the variables kCond and i inside of the main loop, reducing their scope.

This may feel inefficient - If the values are being calculated from scratch every time, are we not being hurt by the fact that multiplication is slower than addition? The answer is no, not really. Both operations can be done hundreds of millions of times per second - and those numbers are from nearly a decade ago.

---

Names: This may not seem important, but the name of this method says nothing about what it's supposed to do, and none of these variables say anything about how it is being done. If you showed me this code without also showing me the output, it might take me several minutes to figure out what the code is for.

At this point, the function now looks like this:

public static void method1()
{
  for (int p = 0; p <= 5; p += 1)
  {
    int kCond = 6 - p;
    PrintMultiple(" ", kCond);

    int inc = p * 2 + 1;
    PrintMultiple("*", inc);

    Console.WriteLine();
  }
}

And some possible names for the variables become clear:

public static void PrintPyramid()
{
  for (int row = 0; row <= 5; row += 1)
  {
    int numberOfSpaces = 6 - row;
    PrintMultiple(" ", numberOfSpaces);

    int numberOfStars = row * 2 + 1;
    PrintMultiple("*", numberOfStars);

    Console.WriteLine();
  }
}

I hope you'll find that our series of seemingly-unimportant changes resulted in code that is both highly readable and acceptably performant.