3
\$\begingroup\$

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?

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! This is a good first question and I’m sure people here will help. \$\endgroup\$ – Edward Dec 9 '19 at 22:57
5
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ It's quite silly to micro-optimize beginner-level programs like this - none will benefit from that. The correct optimized solution is otherwise to make a compile-time script which generates the following C source: godbolt.org/z/oq8Hej. And behold, it outperforms your malloc example by executing many thousand times faster... \$\endgroup\$ – Lundin Dec 10 '19 at 15:17
  • \$\begingroup\$ @Lundin depends on what you're optimizing for. Both your code and your binary take up more disk space and are unmaintainable. My proposal is a happy medium between maintainability, simplicity and reasonable runtime. It's broad optimization but certainly not micro-optimization. \$\endgroup\$ – Reinderien Dec 10 '19 at 18:34
  • \$\begingroup\$ My suggestion for printing strings and for using recursion was for clarity, rather than optimization. But I know that recursion in a non-functional language is not always considered clear, so the answer was deleted. \$\endgroup\$ – jxh Dec 10 '19 at 20:47
0
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$
-3
\$\begingroup\$

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
*
**
***
****
*****
******
*******
********
*******
******
*****
****
***
**
*
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ I like the question and I like the answer, why is the answer downvoted ? Please explain. \$\endgroup\$ – BobRun Dec 10 '19 at 0:42
  • 4
    \$\begingroup\$ @BobRun The answer provides no explanation as to why it's an improvement to the OP's code. Code only answers aren't really accepted by the community. \$\endgroup\$ – Linny Dec 10 '19 at 0:58
  • 2
    \$\begingroup\$ While I respect your opinion, I believe we learn a lot by simply reading code, and in this case reading the code is more efficient than a lot of explanations. \$\endgroup\$ – BobRun Dec 10 '19 at 1:42
  • 5
    \$\begingroup\$ "I added in the "goes-to" operator just for fun" please don't. This will just confuse beginners, which the OP seems to be. Especially if you don't explain why the "goes-to" ""operator"" works. \$\endgroup\$ – Quintec Dec 10 '19 at 1:57
  • 2
    \$\begingroup\$ I agree that goes-to and the answer structure are both problematic. That said - even though the OP didn't ask specifically about performance, this memset method will probably outperform individual putchar calls. \$\endgroup\$ – Reinderien Dec 10 '19 at 4:23

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.