ASCII-art pyramid

Question

I am studying C while listening to a lecture by myself. When I was studying coding and listening to a lecture, I heard that writing code should be concise and maintainable. But I didn't know if I was doing well, so I felt the need for feedback. But it's hard to find anyone who can give us feedback, and I don't know where to ask.

I am Korean and I am using a translator because I am not good at English. Please understand if the context feels a little strange

For example How do you make this code simpler and more intuitive?

Output

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

Program:

#include<stdio.h>

int main(void)
{
    int i, j;
    int n = 5;

    for (i = 1; i <= n; ++i)
    {
        for (j = 1; j <= i; ++j)
            putchar('*');

        putchar('\n');
    }

    for (i = n - 1; i >= 1; --i)
    {
        for (j = 1; j <= i; ++j)
            putchar('*');

        putchar('\n');
    }


    return 0;
}

Would it be better to write if? Would it be better to have a variable name other than n?

Answer 1

A function to print a line of stars of some length seems like a good idea to me.

#include <stdio.h>

static void print_star_line(int count)
{
    int i;

    for (i = 1; i <= count; ++i)
        putchar('*');

    putchar('\n');
}

int main(void)
{
    int i;
    int n = 5;

    for (i = 1; i <= n; ++i)
        print_star_line(i);

    for (i = n - 1; i >= 1; --i)
        print_star_line(i);

    return 0;
}

Notice how it’s clearer at a glance what’s happening in main, and that the shared logic is in one place so it can be examined on its own. Plus, if you ever want to make changes to how a line is printed, you only have to make those changes once.

Apart from that, in C89, that’s about as good as it gets! Your code is pretty clean. Well done.

Answer 2

The issue of performance hasn't specifically been raised, but it's worth addressing (for giggles):

Individual calls to putchar, while programatically simple, are slower than sending out entire strings at a time from a buffer. @jxh had posted an answer using basically this principle, though I wouldn't make it recursive since a loop is perfectly fine and won't blow your stack.

To illustrate the difference: using your original code (but with n=20000), I get

$ gcc -std=c18 -Wall -O3 -march=native -o slow pyramid-slow.c
$ time ./slow > /dev/null
real    0m0.832s
user    0m0.808s
sys 0m0.023s

Using the same size but puts-based code, we get

$ gcc -std=c18 -Wall -O3 -march=native -o fast pyramid-fast.c
$ time ./fast > /dev/null

real    0m0.058s
user    0m0.039s
sys 0m0.019s

The puts code is:

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(void) {
    const int n = 20000;

    char *buf = malloc(n + 1);
    assert(buf);
    memset(buf, '*', n);
    buf[n] = '\0';

    int i;
    for (i = n-1; i >= 0; i--)
        puts(buf + i);
    for (i = 1; i < n; i++)
        puts(buf + i);

    return 0;
}

This code offers a fourteen-fold speedup for the shown size. The reason for this is that printing to the screen is effectively a file operation, and file operations are faster when they operate on large blocks of memory instead of on one byte at a time.

All of this should be taken with a grain of salt, since it's nearly guaranteed that performance is not a concern for your application. Even so, this method avoids having to write nested loops and is thus more readable, as long as you can wrap your head around the loop indexing.

Answer 3

In addition to the remarks by Ry-, it is good practice to keep for loops trivial. Trivial in this context means for(int i=0; i<n; i++). This is the purest and most readable form of for loop.

• It is praxis to always iterate from 0 to n whenever possible, since numbers start with 0 and arrays in C use zero-indexing.
• Downcounting for loops or for loops with complex condition/iteration expressions are harder to read.
• (Advanced) Up-counting, simple loops are more likely to be data cache-friendly than other loops.

We can move the complexity inside the for loop body instead (I modified the code posted by Ry-):

#include <stdio.h>

static void print_star_line (int count)
{
  for (int i=0; i<count; i++)
  {
    putchar('*');
  }
  putchar('\n');
}

int main (void)
{
  int n = 5;

  for(int i=0; i<n; i++)
  {
    print_star_line(i+1);
  }
  for(int i=0; i<n; i++)
  {
    print_star_line(n-i-2);
  }

  return 0;
}

Now all loops are trivial and easy to read, the complexity lies in the expression passed on to the function instead.

---

Note that this code also places the int declarations inside the for loop, which is how code should be written in standard C.

If your source of learning is hopelessly outdated, someone might teach you to use the 30 years old, 20 years obsolete "C90" standard, which doesn't allow this. If so, you need a newer source of learning.

---

The variable name n is fine though vague. It is common style to use n or a _N suffix when counting something.

Similarly, i stands for iterator and is the industry standard name for a loop iterator. j means nothing, it's just the letter that follows i in the alphabet, so it is commonly used when nesting loops.

Answer 4

printf already has the ability to print various lengths of stars, using the formatter %.*s. In this code, I avoid multiple nested loops by simply calculating how many stars I want on each line, and passing that number to printf.

I think this code is much shorter and simpler than nested for-loops.

I added in the "goes-to" operator just for fun:
eg: while (n --> 0), which is read as, "while N goes to zero"

#include <stdio.h>

int main(void) {
    int n = 8;
    char stars[n];
    memset(stars, '*',n);

    int i = n;
    while(i --> 0)
        printf("%.*s\n", n-i, stars);

    while(n --> 0)
        printf("%.*s\n", n, stars);

    return 0;
}

Output

Success #stdin #stdout 0s 4460KB
*
**
***
****
*****
******
*******
********
*******
******
*****
****
***
**
*