5
\$\begingroup\$

I'm a complete beginner, but I'd like to learn how to program. I really started 2 weeks ago.

So I tried the common exercise, aka the multiplication table, but I tried to add little things I learnt recently in order to challenge myself, like functions, tables and pointers even if it's useless here.

The code works well, but I have some warnings when compiling, and I don't know why, like *pointSize = NULL;

Besides, I'm sure my code could be cleaned up, so I'd like you to tell me how I can improve it.

Here's the result: enter image description here

/* Multiplication table */

#include <stdio.h>
short tableSize; // Number of multipliers
short question (short *pointSize); // Ask user how many multipliers
void error(short nbUser); // Cherck error
void multTable(); // The table

void main ()
{
    short i = 1, nbMultipliers;
    question(&nbMultipliers);
    error(&nbMultipliers);
    tableSize = nbMultipliers;

    /* header */
    printf("       |");
    for (i = 1; i <= tableSize; i++)
    {
        printf("  %3u  |", i);
    }
    printf("\n");

    for (i = 1; i <= (tableSize + 1); i++)
    {
        printf ("========");
    }
    printf("\n");
   /* end header */

    multTable(); // the table
}

/* Ask user how many multipliers */
short question (short *pointSize)
{
    *pointSize = NULL;

    printf("How long this table should be ? (0 - 50) ");
    scanf("%u", pointSize);
    printf("\n");
}

void error (short nbUser)
{
    if (nbUser < 0 || nbUser > 50)
    {
        printf("Error : you must enter a number between 0 and 50");
    }
}

/* end Ask user how many multipliers */

void multTable()
{
    short i = 1, j = 1;
    short line[50] = {0};

    for (j = 1; j <= tableSize; j++)
    {
        printf("  %3u  I", j);
        for (i = 1; i <= tableSize; i++)
        {
            line[i] = i*j;
            printf ("  %3u  |", line[i]);
        }
        printf("\n");
        for (i = 1; i <= (tableSize + 1); i++)
        {
            printf ("-------+");
        }
        printf("\n");
    }

}
\$\endgroup\$

closed as off-topic by 200_success, dfhwze, yuri, IEatBagels, Vogel612 Jun 21 at 16:37

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – 200_success, dfhwze, yuri, IEatBagels, Vogel612
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 10
    \$\begingroup\$ Please don't post an image when text is perfectly fine. \$\endgroup\$ – Deduplicator Jun 20 at 22:13
  • 1
    \$\begingroup\$ This code can't possibly work, the way you call the error(short nbUser) function witherror(&nbMultipliers). \$\endgroup\$ – 200_success Jun 21 at 5:46
10
\$\begingroup\$

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 #included. 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 printfs: 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 shorts. 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.

\$\endgroup\$
  • \$\begingroup\$ I really appreciate you took some time to make a clear answer. I compile directly in Visual Code and sometimes GCC in a terminal. They both use GCC but don’t always react the same way. I didn’t know about -Wextra, thanks. I used short ints because I will use C mainly to program an Arduino so I try to optimize as much as I can, but it seems I did it the wrong way. \$\endgroup\$ – Nevralgeek Jun 21 at 18:15
2
\$\begingroup\$

Good start mate. Few items for you to consider:

  • Things tend to get complicated by law of entropy all by them self - your help is not needed (aka: KISS)
  • Use natural names so your code reads as close to English as possible.
  • Learn to be a good listener - if an experienced user tells you huge files, like pictures are not appreciated needlessly, take pain and replace them by text file, as suggested.
  • Edit yourself first, so there is less for others to do, you stated that the posted code works, however it did not compile as pasted, make sure you post the working version by running it just before posting and copying from correct file.

And here is a bit cleaner version:

    /* Multiplication table */
    #include <stdio.h>
    #define MAX_SIZE 31

    int getSize();
    void printHeader(int);
    void printTable(int);

    int main ()
    {
        while(true)
        {
            int size = getSize();
            if (size == 0) break;
            printHeader(size);
            printTable(size);
        }
        return 0;
    }

    int getSize()
    {
        int size = -1;
        while (0 > size || size > MAX_SIZE)
        {
            printf("\nPlease enter number between 1 and %d to specify \nmultiplication table size, or 0 to exit: ", MAX_SIZE);
            scanf ("%d", &size);
            printf("\n");
            if(0 > size || size > MAX_SIZE) printf("Error, please check your entry and try again!");
        }
        return size;
    }

    void printHeader(int size)
    {
        printf("       ┃");
        for (int i = 1; i < size + 1; i++)
            printf("  %3u  ┃", i);
        printf("\n");
        for (int i = 1; i < size + 2; i++)
            printf ("━━━━━━━╋");
        printf("\n");
    }

    void printTable(int size)
    {
        for (int j = 1; j <= size; j++)
        {
            printf("  %3d  ┃", j);
            for (int i = 1; i < size + 1; i++)
                printf("  %3d  |", i * j);
            printf("\n");
            for (int i = 1; i < size + 2; i++)
                printf ("━━━━━━━╋");
            printf("\n");
        }
    }

And here are few sample input/outputs:

    Please enter number between 1 and 31 to specify 
    multiplication table size, or 0 to exit: 0

$ ./multip

    Please enter number between 1 and 31 to specify 
    multiplication table size, or 0 to exit: 5

           ┃    1  ┃    2  ┃    3  ┃    4  ┃    5  ┃
    ━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋
        1  ┃    1  |    2  |    3  |    4  |    5  |
    ━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋
        2  ┃    2  |    4  |    6  |    8  |   10  |
    ━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋
        3  ┃    3  |    6  |    9  |   12  |   15  |
    ━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋
        4  ┃    4  |    8  |   12  |   16  |   20  |
    ━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋
        5  ┃    5  |   10  |   15  |   20  |   25  |
    ━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋━━━━━━━╋

    Please enter number between 1 and 31 to specify 
    multiplication table size, or 0 to exit: 77

    Error, please check your entry and try again!
    Please enter number between 1 and 31 to specify 
    multiplication table size, or 0 to exit: 

Please do not forget to mark as answer and vote up if you appreciate my help. And for extra bonus, check out this Box Drawing Reference and make the table perfect by using the 8 corner piece characters I should have but did not.

\$\endgroup\$
  • 5
    \$\begingroup\$ It's probably not a good idea to use "block quotes" for parts of the answer YOU wrote. They are meant for, wait...wait, quotes. \$\endgroup\$ – AlexV Jun 21 at 11:46
  • \$\begingroup\$ Thanks @AlexV - you are 100% right, done, and I would keep it in mind for future. \$\endgroup\$ – Lucifer Morningstar Jun 23 at 1:49

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