7
\$\begingroup\$

I wrote a code which prints the following pattern :

5 5 5 5 5 5 5 5 5 
5 4 4 4 4 4 4 4 5
5 4 3 3 3 3 3 4 5
5 4 3 2 2 2 3 4 5
5 4 3 2 1 2 3 4 5
5 4 3 2 2 2 3 4 5 
5 4 3 3 3 3 3 4 5 
5 4 4 4 4 4 4 4 5
5 5 5 5 5 5 5 5 5

The code is as follows :

#include <stdio.h>

void main(){
   int n,i,k,b,a;
   printf("ENTER OUTER NUMBER:");
   scanf("%d",&n);
   for(i=0;i<2*n-1;i++){
     printf("%d",n);
   }
   printf("\n");
   int p=n;
   for(i=0;i<n-1;i++){
     int a=0;
     for(k=0;k<=i;k++){
        printf("%d",n-a);
        a++;
     }
     for(b=0;b>i;b++){
        printf(" ");
     } 
     for(k=1;k<=2*p-3;k++){
        printf("%d",n-i-1);
     }  
     p--;
     for(k=i;k>=0;k--){
        printf("%d",n-k);
     }
     printf("\n");
   }
    int q=1;
    for(i=0;i<n-2;i++){
     for(k=n;k>i+1;k--){
        printf("%d",k);
     }
     for(k=1;k<=2*q-1;k++){
        printf("%d",q+1);
     }
     q++;
     for(k=2+i;k<=n;k++){
        printf("%d",k);
     }
     printf("\n");
   }
   for(i=0;i<2*n-1;i++){
     printf("%d",n);
   }
   getchar();
}

I printed the first and last line independently.Then,I divided the pattern into two halves and considered some common triangular patterns as follows:

5                 5    2 3 4 5 
5 4             4 5      3 4 5
5 4 3         3 4 5        4 5
5 4 3 2     2 3 4 5          5 

The code ran well in the CODE BLOCKS IDE.But, I think it's too long and I complicated the coding.My query is:

  1. Can I shorten the code with the similar logic of breaking the pattern into parts or mine is ok ?

  2. Is there any alternative to this ?

\$\endgroup\$

4 Answers 4

6
\$\begingroup\$
  • Use a bit more spaces. It will make your code more readable.

  • Try to be coherent with your indents

  • You should sanitize input. What happen if user enter a number greater than 9 ? Or 0, 1, or 2? Or negative number? Or not a number? Never trust user, They all try to broke your program.

  • When you just print on char, prefer putchar() instead of printf().

Here's my solution, maybe not the most optimal, but clean and short. I compute the distance from the current coordinate to nearest side and then remove it to the base number.

#include <stdio.h>
int main(void) {
    int n = 5;
    int a;
    int b;
    const int m = 2*n-1;
    for (int i = 0; i < m; ++i) {
        for (int j = 0; j < m; ++j) {
            a = (i >= n) ? m - i - 1: i;
            b = (j >= n) ? m - j - 1 : j;
            putchar('0' + n - ((a < b) ? a : b));
        }     
        putchar ('\n');
    }
    return 0;
}

To improve this, you could split the inner loop to avoid the two conditional assignments. I let you trying to do it yourself.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Little reason to declare a, b so early. Consider defining when needed: a = (i >= n) ? m - i - 1: i; --> int a = (i >= n) ? m - i - 1: i; \$\endgroup\$
    – chux
    Commented Aug 7 at 21:12
  • 1
    \$\begingroup\$ a = (i >= n) ? m - i - 1: i; --> consider moving to outside the for (int j = 0; j < m; ++j) loop. \$\endgroup\$
    – chux
    Commented Aug 7 at 21:13
3
\$\begingroup\$

My first impression is that the code is very hard to read.

There are lots of variables all with single-letter identifiers, without even comments to help understand what each one represents. As a general rule, a variable's descriptiveness should be proportional to the size of its scope.


void main() is not a portable declaration of main() - if your compiler doesn't at least warn about this, you probably haven't enabled enough diagnostics. Portable definitions of main() all return int (though you don't explicitly need to return a value - reaching the end of main() will cause the runtime to exit with a success status value).


Variable int a is declared but never used.


If printf("ENTER OUTER NUMBER:") fails, it seems foolish to try reading input. Don't ignore function return values unless you've considered the consequences!

Even more significantly, if the scanf() call fails to convert its input to int, then we should not be continuing with uninitialised value of n.

Consider accepting the size as a command-line argument, rather than requiring it to be passed via standard input. You could use the existing logic as a fallback if the argument isn't provided.

Having said that, we should think about what range of n we're willing to accept. Consider this invocation:

./207421 <<<12

ENTER OUTER NUMBER:1212121212121212121212121212121212121212121212
1211111111111111111111111111111111111111111112
1211101010101010101010101010101010101010101112
12111099999999999999999101112
12111098888888888888889101112
12111098777777777777789101112
12111098766666666666789101112
12111098765555555556789101112
12111098765444444456789101112
12111098765433333456789101112
12111098765432223456789101112
12111098765432123456789101112
12111098765432223456789101112
12111098765433333456789101112
12111098765444444456789101112
12111098765555555556789101112
12111098766666666666789101112
12111098777777777777789101112
12111098888888888888889101112
12111099999999999999999101112
1211101010101010101010101010101010101010101112
1211111111111111111111111111111111111111111112
1212121212121212121212121212121212121212121212

If we're to assume that each number has width of 1 character, then we need to ensure 0 < n < 10.

Alternatively, we could specify a field width when we printf() each value, so that all values are printed consistently wide. We could obtain the required width for the largest value using snprintf(NULL, 0, …). Even then, we might to limit the width for usability reasons - particularly if standard output is connected to a terminal!


Splitting the output up over several loops makes it very hard to follow. I'd prefer to use a single pair of nested loops to write all the output:

    for (unsigned y = 0; y < width;  ++y) {
        for (unsigned x = 0;  x < width;  ++x) {
            printf("%u ", distance_from_centre(size, x, y));
        }
        puts("");
    }

All we have to do is write a suitable distance_from_centre function that returns the ring number for a given position.


Improved code

Applying all the suggestions above, we get:

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

/* Read a line from standard input, removing the final newline.
   Returns number of characters read, zero on failure.
 */
static size_t read_string(char *buf, size_t buf_size, char const *prompt)
{
    if (printf("%s: ", prompt) < 2) {
        return 0;
    }
    fflush(stdout);
    if (!fgets(buf, sizeof buf_size, stdin)) {
        return 0;
    }
    size_t len = strlen(buf);
    if (len == 0 || buf[len-1] != '\n') {
        return 0;
    }
    buf[len-1] = '\0';
    return len;
}

/* Find which ring is at the position (x, y) for a set of concentric
   rings where 1 is the inner point and rings is the outer ring.
 */
static unsigned distance_from_centre(unsigned rings, unsigned x, unsigned y)
{
    unsigned dx =  x >= rings  ?  x - rings + 2  :  rings - x;
    unsigned dy =  y >= rings  ?  y - rings + 2  :  rings - y;
    return dx > dy ? dx : dy;
}

/* Convert arg to number, and print that many concentric rings */
static unsigned print_rings(char const *arg)
{
    /* Convert input; allow only 1-99 rings */
    char *end;
    unsigned long const rings_l = strtoul(arg, &end, 10);
    if (*end || rings_l - 1 >= 99) {
        fprintf(stderr, "Invalid ring count %s\n\n", arg);
        return 0;
    }
    unsigned const rings = (unsigned)rings_l;

    /* Do the printing */
    int const field_width = snprintf(NULL, 0, "%u", rings);
    unsigned const width = 2 * rings - 1;

    for (unsigned y = 0; y < width;  ++y) {
        for (unsigned x = 0;  x < width;  ++x) {
            printf("%*u ", field_width,
                   distance_from_centre(rings, x, y));
        }
        puts("");
    }
    puts("");
    return rings;
}


int main(int argc, char **argv)
{
    if (argc < 2) {
        /* No arguments: fall back to reading from input */
        char buf[10]; /* plenty for valid sizes, including newline and null */
        if (!read_string(buf, sizeof buf, "Enter number of rings")) {
            fprintf(stderr, "Failed to read input\n");
            return EXIT_FAILURE;
        }
        print_rings(buf);
    } else {
        while (*++argv) {
            print_rings(*argv);
        }
    }
}

You'll notice that much of this code is for robust input and other error-checking; the actual printing code is relatively small. That tends often to be the case when writing C programs.

\$\endgroup\$
1
\$\begingroup\$

The same pattern can be produced by a single instance of nested for loops, where outer loop variable is line and inner one - horizontal position.

I do not want to spoil your fun in finding the function at the heart of the loop (at least yet), which given two coordinates produces the value at those coordinates.

\$\endgroup\$
2
  • \$\begingroup\$ Did you speak about the method as mentioned in the abovw answer? \$\endgroup\$ Commented Nov 14, 2018 at 2:25
  • \$\begingroup\$ Yes. The method above is that. \$\endgroup\$
    – Roman Susi
    Commented Nov 14, 2018 at 4:07
1
\$\begingroup\$

I am sure this is not the shortest answer but I want to provide an answer that is easy for even beginners to easily grasp. Instead of dividing the pattern into triangle, you can divide them into quarters.

My logic is simple. Assume you are writing a code to print n... 5 4 3 2 1 2 3 4 5... n in every line. But then also compute a minimum value for each row.

So if val < min, print min instead. This minimum changes from n -> 1 -> n again.

The long way of doing this:

for(int i = n; i >= 1; i--) {
    for(int j = n; j >= 1; j--) {
        printf("%d ", j < i? i : j;
    }
    for(int j = 2; j <= 2; j++) {
        printf("%d ", j < i? i : j;
    }
    printf("\n");
}
for(int i = 2; i <= n; i++) {
    for(int j = n; j >= 1; j--) {
        printf("%d ", j < i? i : j;
    }
    for(int j = 2; j <= 2; j++) {
        printf("%d ", j < i? i : j;
    }
    printf("\n");
}

Short way:

int n, m;
scanf("%d", &n);
int m = 2*n - 1;
for(int i = 1; i <= m; i++) {
    int min = (i <= n)? n - i + 1: i - n + 1;
    for(int j = 1; j <= m; j++) {
        int val = (j <= n)? n - j + 1: j - n + 1;
        printf("%d ", val >= min? val : min);
    }
    printf("\n");
}
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ Commented Aug 3 at 7:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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