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.