10
\$\begingroup\$

A small project I recently started. The program must sum all the numbers the user input. You must first specify how many numbers there will be.

Sumaster.c

#include <stdio.h>

int main(void)
{
    unsigned short int num = 0;

    while(num <= 0 || num >= 256)
    {
        printf("Enter number of values to sum: ");
        scanf("%d", &num);
    }

    unsigned long long int arr[num];
    unsigned long long int total = 0;
    unsigned short int i;

    for(i = 0; i < num; i++)
    {
        scanf("%lld", arr + i);
        total += arr[i];

        if(i < num - 1) printf("+\n");
        else printf("=\n%lld\n", total);
    }

    return(0);
}
\$\endgroup\$
-1
\$\begingroup\$

Small programs as such should be relatively honored, since they are a lot used as a practical examples, usually for beginners. So an appropriate review follows to be a Nobel improvement attempt of this particular variation.


Name of program

The name of a program like that must be exclusively identical with its functional behavior.

It must hint the notion as clear as possible to the user, before he wastes efforts and resources to open the "mysterious source file". The name you picked up... "Sumaster" is somehow idiosyncratic and certainly not as pretty self-explaining. It could be interpreted in many sick ways. If it tries to exalt the program by calling it "Summation Master" then it applies, but it goes a bit funny if you assimilate it.

My suggestion is simply "Sumilizer" or something a bit more simplified "Auto Summation" (enthusiasts can call it "The A.S Bot" to bring some 21s software eminence affection).

Besides that, if you are in trouble figuring a name, you can visit this powerful international internet cyber searcher and type "Good names for a ..." where ... is the target object you are seeking a good name for.

Program description

...which is missing. The description must provide some additional information about the program usage. If you will be the only one using this, there is obviously no need (except that time when we forget). If someone wants to use the program, he might be unable to guess the unique functional mechanism you invented. Tell me: how many times you got in trouble understanding how one program works?

There is no need for you to dedicate a webpage or a readme.txt. The readme.txt can be in the source file. If this is intended to be open-source. Unlikely it won't be. If this program's future involves beginners, learning from the program, then it will be kind of you to add some description. If it is for your own experience then you can decide. In each circumstance, it isn't completely irrelevant.

/* Summation automatized program by Genis
 * Copyright: Free For Any Purposes
 * Compilation arguments: std=c99
 * 
 * The first thing you'll have to specify is the length of the number table.
 * Enter to continue..
 * Then you are typing the first number you wish to add to the number table.
 * Enter to continue.. then the next number.
 * When the numbers reach the length of the number table, the program will output the total of all the numbers in the number table.
 */

Internationalization

Assuming that this program will be widely used by people around the world, it could be internationalized (i.e translated to a number of regional languages). The MinGW encoding defaults to UTF-8, the char datatype is set to be capable of representing any member of the basic execution character set and UTF-8 code units. Unicode Programming allows you to use larger set of characters.

Variable type-definition

What have you accomplished invokes:

  • Readability issue.
  • Portability Issue.

More pointless additional words and more time required for the user to read and digest the formed data type.. I suggest you "typedef" these data type creations.

typedef unsigned short int uint_16, word;
typedef unsigned long long int uint_32, dword;

And then use these definitions instead. Use word and dword if the program is intended to work under Windows.

Variable naming

Readability issue #1.

  • The first name of the first variable num is not accurate. If someone (or you in the forgetfulness future) wants to edit/analyze the code, he/she will need more time to assimilate the exact purpose of the variable. Why not the name arrSize since it behaves like index and it is an index. Alternatives are table_length for snake_case or TableLength / tableLength for CamelCase name styling.
  • The second name of the second variable arr will be fine if the first variable is named arrSize. That is if we want to name the variables by their behavior, not logical purpose. Otherwise.. you can choose table.
  • total or tableTotal is equally good.

Inconsiderate conversion

As we found out, num is a variable type unsigned short. On line 10 we except signed int as an input for num. This will wake up some compiler optimizations, possibly -Woverflow and it might be quite an undesired behavior after all. In the name of the sense, you can use the printf format specifier "%hu" which will equalize the expectations.

Problematic function termination

If you copy/paste this code into "Skype". return(0) will turn into a clock. It will look like you are returning a clock. Get rid of the brackets! Just kidding.

Overall

I have seen better implementations of a summation. I would prefer to use space in order to move to the next number from the table. And everything happens in one-line instead of being able to pass an unlimited amount of newlines during the process of scanf.

\$\endgroup\$
  • 5
    \$\begingroup\$ typedef unsigned short int uint_16, word; Huh? Why wouldn't you just use uint16_t from stdint.h? And calling an unsigned short a word is not only confusing in the context of this program (typedefs should describe semantics, not implementation), it's not a word on any likely system (WORD on Windows is a bit of a lie due to history). Also, num is not an index, but rather the size of an array, thus the name index would be very confusing (much more confusing than num). \$\endgroup\$ – Corbin Jan 28 '15 at 5:02
  • 4
    \$\begingroup\$ There are things that I disagree entirely in this review. Defining a word and dword doesn't make any sense! Specially since this is to teach people. Also, you forgot to mention the lack of comments and the if's without {}. About i18n, just forget it... This code only deals with numbers. Why would it need such boilerplate? It's just simple numbers. The suggestion to rename num to index is a really bad one. I would like to vote positively on this one, but a good structure doesn't mean accurate content. \$\endgroup\$ – Ismael Miguel Jan 28 '15 at 10:39
  • \$\begingroup\$ @Corbin yes. num is size of the array. I should edit that. word is windows-specific implementation of unsigned short int and there is no need to include an entire library only for one definition. \$\endgroup\$ – Edenia Jan 28 '15 at 11:51
  • 3
    \$\begingroup\$ I think you've picked the wrong supporting argument for why return(0) is bad. Why would anyone mangle code via skype? \$\endgroup\$ – Pimgd Jan 28 '15 at 13:25
11
\$\begingroup\$

It's good overall, and for a toy or class program this is adequate, but it's been a while since I've done a review, so it's time to get super picky :).


There is a very major problem in that the IO handling is wrong. Try firing up your program, and instead of a valid number of items to sum, put in something non-numeric. It will infinite loop.

In short, scanf doesn't move forward in a stream when it doesn't consume anything1. You'll need to either read a line at a time (fgets) and parse an integer out of the line, or you'll need to consume the rest of the line and try again (the easiest approach to that would be to loop on getline until you get a \n).

In a similar vein, your input reading in the loop to collect inputs is just wrong. All it takes is a single incorrect input and the program falls apart.

1 It might be worth noting at this point that scanf has other problems as well.


There's also some portability issues. Before C99, variable length arrays are not required to be supported by the standard, and after C99, they're standardized but not required. This means that C99 is the only standard that requires VLA. That's a pretty precarious situation to put yourself in.

In case you're not sure what I'm talking about, arr is the variable length array since num is not a compile-time constant (there's magic going on under the hood — it's not a normal array, the compiler is just hiding that).

(Realistically, every compiler you're going to come across is going to support this. Just one of the many gotchas in C with regards to portability if you're ever going to experience a truly rare environment.)


I don't understand why the array and sum are unsigned. Seems the same functionality would work just as well if you were to allow signed numbers? The only reason I can think of for using unsigned values is if you're trying to eke out a little more range, but that's probably misguided and if a 64 bit integer isn't large enough, you're probably into arbitrary precision land.

In short, I think all of your unsigned long long stuff should just be long long int or int64_t (from stdint.h). Even though long long int is actually guaranteed to be 64 bits or more whereas int64_t is required to be exactly 64 bits (and is not required to be supported), I think int64_t is clearer to read (and realistically, if long long exists, int64_t is going to exist).


This is mildly controversial, but I'm not really a fan of short unless you're doing it because you actually need a specific size (memory constrained stuff, you're going to have a lot of them, alignment issues, etc). ints come across a bit more naturally to work with, and depending on CPU architecture, a short is probably being treated as an int anyway.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would actually suggest to use a 32-bit long unsigned variable. Most systems (specially at schools) have 32-bit Operating Systems. Since this is for demonstrations purposes, we must stick with it. \$\endgroup\$ – Ismael Miguel Jan 28 '15 at 10:32
3
\$\begingroup\$

You should always compile with warnings enabled. A good compiler should have warned you…

$ clang -Wall -o sumaster sumaster.c
sumaster.c:10:21: warning: format specifies type 'int *' but the argument has
      type 'unsigned short *' [-Wformat]
        scanf("%d", &num);
               ~~   ^~~~
               %hd
1 warning generated.

As a result, I can do this (results will vary, since it's undefined behaviour):

Enter number of values to sum: -2147483645
1
+
2
+
3
=
6

The scanf() function is actually quite difficult to use correctly, especially when error handling needs to be done. For example, what happens in your program when, at the "Enter number of values to sum: " prompt, you hit CtrlD (Unix) or CtrlZ (Windows) to generate an end-of-file condition? What about if you enter A? Both cases result in an infinite loop. One remedy is to rely on fgets() instead; another is to discard any bad input up to the first newline before retrying.

Since an error-tolerant number input routine is rather complicated, it makes sense to define a function for it.


Unsigned numbers are actually tricky to work with. Your num <= 0 comparison, for example, actually behaves as num == 0, since num will never be negative.

Surprisingly, your program will accept and add negative numbers as well (though, once again, it's undefined behaviour that you can't count on).

Due to these deceptive behaviours, you're better off using signed data types (and performing validation if you want to prohibit negative numbers). There is also not much point to using a data type narrower than an int. Compilers will likely pad the short for memory alignment anyway, so you wouldn't save space. A short would only make sense if you're dealing with a binary file or network protocol where the exact size matters (in which case an explicitly sized type like int16_t would be more appropriate anyway) or if you have an array that is large enough so that the space matters.


There isn't any point to storing all of the inputs in an array. You could achieve the same effect by just keeping the running total.


Suggested solution

#include <stdio.h>

/**
 * Prompts the user using the given prompt until a long long int is
 * successfully read.  If the input is not an integer, prints err_msg
 * and prompts the user again.
 *
 * Returns 1 on success, or 0 if EOF is encountered.
 */    
static int prompt_ll(const char *prompt, long long *n, const char *err_msg)
{
    do
    {
        printf("%s", prompt);
        switch (scanf("%lld", n))
        {
          case EOF:
            return 0;
          case 1:
            return 1;
          default:
            scanf("%*[^\n]");   /* Discard the rest of the bad line */
            printf("%s", err_msg);
        }
    } while (1);
}

int main(void)
{
    long long n;
    do
    {
        if (!prompt_ll("Enter number of values to sum: ", &n, "")) return 1;
        if (0 <= n && n < 256) break;
        printf("Enter a number between 0 and 255.\n");
    } while (1);
    int num = (int)n;

    long long total = 0;
    for (int i = 0; i < num; i++)
    {
        if (!prompt_ll(i ? "+\n" : "", &n, "Try again.\n")) return 1;
        total += n;
    }
    printf("=\n%lld\n", total);
}
\$\endgroup\$
  • \$\begingroup\$ The review quality is amazing, but the suggested is too advanced for people who aren't really into C (I had 1 year of classes in C and have some troubles reading some parts of your code). Also, you don't follow good practices: if's without {}, spaces missing, usage of un-explicit variable names (looking at the variable n) and a huge lack of comments. \$\endgroup\$ – Ismael Miguel Jan 28 '15 at 10:30
  • \$\begingroup\$ @Ismael I don't discuss brace styles in reviews, to avoid unproductive controversy. Since you mention it, I've used Allman style to match the original code, despite my personal hatred of it for wasting space. I've therefore put return and break on the same line as a safe way to save three lines per condition. \$\endgroup\$ – 200_success Jan 28 '15 at 10:50
  • \$\begingroup\$ @Ismael As for comments, I've written a detailed one for prompt_ll() where it is needed and a brief one for the enigmatic scanf("%*[^\n]"). Everything else is straightforward. You can infer the purpose of the two stanzas in main() from "Enter number of values to sum: " and the variable name total. \$\endgroup\$ – 200_success Jan 28 '15 at 10:52
  • \$\begingroup\$ I understand why you don't discuss braces, but the lack of it might contribute for really bad, bug-prone code. This happened before! I've read a while ago about a vulnerability in iOS due to someone forgetting to put braces around an if. The accuracy is debatable. Also, I would rename the functon prompt_ll to prompt_line. But I still believe that this is over-complicating. The O.P was writting a simple program, which isn't that simple anymore. \$\endgroup\$ – Ismael Miguel Jan 28 '15 at 10:56
  • \$\begingroup\$ @Ismael I frequently mention goto fail in my reviews, when programmers omit braces for a statement on the next line. Same-line statements should be immune to that kind of failure, though. Unfortunately, the original simple program is not robust. I encourage you to write a review with a better suggestion. \$\endgroup\$ – 200_success Jan 28 '15 at 11:07
1
\$\begingroup\$

There seems to be no reason to use a VLA in this context, you already have specified that the array has max size 255 so why not just declare it as such.

unsigned long long int arr[255];

that way you are more independent from the compiler version/options.

when reading a value from keyboard use instead the more robust fgets to catch the input, even better put it in a function:

e.g.

// read an integer 1..255
// @returns -1 if user wants to quit, 1..255
int getInt()
{
  char line[32];

  while (fgets(line, sizeof(line), stdin) != NULL) 
  {
    if ( sscanf( line, "%d", &num ) == 1 )
    {
      if (num > 0 && num < 256)
      {
        return num;
      }
      puts( "invalid input, please enter a number in range [1,255]" ); 
    }
    else
    {
      puts( "please enter a number" );
    }
  }
  return -1;
}
\$\endgroup\$
0
\$\begingroup\$

I'll keep it simple.

Documentation really does NOT matter here. The intent seems pretty clear.

Sum all integers while using a VLA.

Header is fine. The most complicated thing happening here is I/O.

Return can be cast, it's personal preference.

You can use return 0; or return (0); or return(0);

and if you're really feeling saucy; as horrific as it may seem and startle the community into mass pandemic of terror and anxiety... you can include stdlib.h, void main(void), and use exit(0).

Even if short is 2 bytes, you still have up to a range of -65,537 to 65,536 possible values. Assuming most systems can handle a 4 byte int, long long isn't really necessary. unsigned, in this context, can cause serious issues in even the most basic of concepts such as this one. You should allow for signed values. I'll explain why in a bit. But for now, we'll change this

unsigned short int num = 0;

to this

int num = 0;

The while() loop is good. If num is <= 0 or num is >= 256 prompt the user again. Your filtering the input, but there is a catch with scanf() and its that it reads up until the first whitespace character. You need a way to clear the input.

while (getchar() != '\n'); or while (getchar() != '\n') continue;

usually solves this problem. That way, any invalid input is cleared and new input is ready. Also, you note that while num is less than zero. If your num variable is of type unsigned, you've created a semantic error. One that took me awhile to get used to too. It's incredibly easy to make it as well. That's why it should be signed and not unsigned. unsigned types have their place and this context is not it.

while(num < 0 || num > 255)
{
    printf("Enter number of values to sum: ");
    scanf("%d", &num);
    while (getchar() != '\n') continue; //clear the input buffer
}

as for the vla arr, the total variable, and the i variable...

unsigned long long int arr[num];
unsigned long long int total = 0;
unsigned short int i;

should really be

int arr[num], total, i;

you don't even really need to declare i, but if you want compatibility, you can declare i outside of the for() loop. Again, it depends on context and preference.

again, we don't need a long long type. just an int type.

    scanf("%lld", arr + i);

should really just be

scanf("%d", &arr[i]);

since we are working with arrays, not pointers (even though, an array is a type of pointer), the intent is clearer and consistency runs throughout your code; and remember, were storing the value into that array member.

we don't need to offset num by 1 since i needs to be less than num. The elements are already offset by one and initializing i to 0 accomplishes that goal. but really, we don't even really need that code at all. all we need to do is print the total.

for(i = 0; i < num; i++)
{
    scanf("%d", &arr);
    while (getchar() != '\n') continue; //clear the buffer
    total += arr[i];
    printf(" + ");
}
printf("total sum is %d\n", total);

but you should get the idea by now.

in all, this is my revised code. while, i'll admit, it is very buggy and unstable. its a basic framework to play with and displays my point of clearing whitespace produced by using scanf(). it accomplishes the task of creating a vla and the data types used fall well within the bounds created by the initial input.

assuming vlas are being used, we can assume that a c99 compliant compiler is being used.

#include <stdio.h>

int main(void)
{
    int num = 0;

    printf("Enter number of values to sum: ");
    while(1 != scanf("%d", &num) && (num < 0 || num > 256))
    {
        printf("Enter number of values to sum: ");
        while (getchar() != '\n'); //clear buffer
        scanf("%d", &num);
    }
    while (getchar() != '\n');

    int arr[num], total = 0;

    printf("%d number(s) to be summed: ", num);
    for(int i = 0, j = num - 1; i < num; i++, j--)
    {
        while (1 != scanf("%d", &arr[i]))
        {
            printf("%d number(s) to be summed: ", j);
            while (getchar() != '\n') continue; //clear buffer
            scanf("%d", &num);
        }
        while (getchar() != '\n');
        total += arr[i];
        if (j != 0) printf("%d number(s) to be summed: ", j);
    }
    printf("The total sum is %d\n", total);

    return 0;
}
\$\endgroup\$

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.