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.