You should generally try to avoid global variables (like tableSize
). Since they can be used everywhere ("globally"), at the worst case you need to read all of the code to figure out where they come from and how they're used. In this case, tableSize
could become a parameter of multTable
.
While it will probably work on the compilers you'll come across, void main()
isn't strictly guaranteed to work in all C implementations. The standard signature is int main(int argc, char** argv)
, or possibly int main(void)
.
Defining multiple variables in a single line, like short i = 1, nbMultipliers;
, is generally frowned upon because (a) it can be hard to read, and (b) C made some questionable decisions about the need to repeat qualifiers like const
and *
on types, making the syntax dangerous.
However, you shouldn't really be defining all of your variables at the top of your functions (this was required in old versions of C, but hasn't been for twenty years). You should define your variables as late as possible.
The signature of question
could be simplified to short question(void)
-- don't use pointers when you don't have a reason to.
You are calling error
incorrectly. You've defined it to take a short
, but you're passing it a pointer to a short
. It should instead be called like error(nbMultiplier);
. The C compiler should tell you this -- are you compiling using warnings? The flags -Wall -Wextra
are very useful!
Initializing pointSize
prior to reading in is not necessary, and actually possibly harmful to the compiler/a debugger catching your mistakes. Also, NULL
is a value for pointers. When you're setting *pointSize
, you're setting the value of a short
, not a pointer. You probably mean 0
or -1
or some other value - but again, it's simpler to just leave it uninitialized in this case, so that a sanitizer or debugger can catch any accidental uses of uninitialized data.
The format specifier for short
is %hd
, not %u
. Using %u
will invoke undefined behavior. Here too, the compiler will emit a warning telling you to fix this.
exit
is included in stdlib.h
, which you have not #include
d. It also requires a integer parameter, the exit code to use. Since this is an error, you should use EXIT_FAILURE
.
Here is what your program looks like (skipping the body of multTable
which we haven't gotten to yet) if you apply all of the above:
#include <stdio.h>
#include <stdlib.h>
short question(void); // Ask user how many multipliers
void error(short nbUser); // Cherck error
void multTable(short); // The table
int main(void)
{
short tableSize = question();
error(tableSize);
/* header */
printf(" |");
for (short i = 1; i <= tableSize; i++)
{
printf(" %3u |", i);
}
printf("\n");
for (short i = 1; i <= (tableSize + 1); i++)
{
printf("========");
}
printf("\n");
/* end header */
multTable(tableSize); // the table
return 0;
}
/* Ask user how many multipliers */
short question(void)
{
short pointSize;
printf("How long this table should be ? (0 - 50) ");
scanf("%hd", &pointSize);
printf("\n");
return pointSize;
}
void error(short nbUser)
{
if (nbUser < 0 || nbUser > 50)
{
printf("Error : you must enter a number between 0 and 50");
exit(EXIT_FAILURE);
}
}
void multTable(short tableSize)
{
......
}
Notice how related question()
and error()
; you use error()
only to evaluate the output of question()
, and question()
's output cannot be trusted without invoking error()
. Instead of making question()
possibly return a bad value, you should make it encapsulate the error detection itself:
short question(void)
{
short pointSize;
printf("How long this table should be ? (0 - 50) ");
scanf("%hd", &pointSize);
printf("\n");
if (pointSize < 0 || pointSize > 50)
{
printf("Error : you must enter a number between 0 and 50");
exit(EXIT_FAILURE);
}
return pointSize;
}
Also, notice that you have two places that have printf
s: both main
and multTable
. It's good practice to keep each function doing a single understandable thing. main
is doing too much in also printing the table header; especially since that table header needs to be changed if you ever change multTable
's output format. So, you should move printing the table header into multTable
.
In multTable
, look carefully at how you're using the line
variable. You're only using it to write to line[i]
, and then immediately read back line[i]
-- you're not using it as an array at all! It can simply be replaced with a local short
variable.
The resulting multTable
will look something like this:
void multTable(short tableSize)
{
/* header */
printf(" |");
for (short i = 1; i <= tableSize; i++)
{
printf(" %3u |", i);
}
printf("\n");
for (short i = 1; i <= (tableSize + 1); i++)
{
printf("========");
}
printf("\n");
/* end header */
for (short j = 1; j <= tableSize; j++)
{
printf(" %3u I", j);
for (short i = 1; i <= tableSize; i++)
{
short product = i * j;
printf(" %3hd |", product);
}
printf("\n");
for (short i = 1; i <= (tableSize + 1); i++)
{
printf("-------+");
}
printf("\n");
}
}
General comments
First, you should put more care into how you name things.
You should generally avoid abbreviations (unless they are extremely standardized abbreviations). You read all the time, & u dnt gnly rd txtlkths
, you read text that looks like this
. Don't say nb
, say number
. Don't say mult
, say multiplication
.
Functions should generally be named like commands to do something. For example, printMultiplicationTable()
. This lets you read code as a sequence of commands, each one saying what it does. Look at how clear and simple the following looks:
int main(void) {
short tableSize = getTableSize();
printMultiplicationTable(tableSize);
return 0;
}
Comments should not state things that are obvious. A comment of // the table
adds absolutely nothing to a line that consists of multTable();
. Also, you generally don't need /* end .. */
comments. Your text editor or IDE can let you quickly jump to the beginning and end of functions, and "collapse" them. Keeping track of these manually in the code adds very little value but quickly adds a lot of clutter.
You also hardly need to use short
s. They are often dangerously small (real world numbers very quickly get larger than 16,000), and are no longer faster to operate on in modern computers. If you have a very large array of small numbers, then it might be time to use a short
instead of an int
(though an explicitly sized type, like int16_t
, would be even better!)
Your multiplication table accepts input up to 50
, but it won't actually work properly for inputs that large. That's because the largest numbers in the resulting table are more than 3 digits wide. Your code should use 4 columns, or you should dynamically adjust the number of columns depending on what's needed for the user's input.
Also, it bears repeating, make sure you compile with compiler warnings on, and address all of the warnings. C is not easy to get right, and C compilers generally give very helpful warnings that catch lots of mistakes that both beginners and veterans make. You should probably use both -Wall
and -Wextra
if you're using clang
or gcc
to get all of the warnings turned on.
error(short nbUser)
function witherror(&nbMultipliers)
. \$\endgroup\$