Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
}
share|improve this question

3 Answers 3

up vote 1 down vote accepted

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.

share|improve this answer
    
Thanks for the quick and detailed answerr!!! –  Genis 13 hours ago
3  
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). –  Corbin 10 hours ago
2  
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. –  Ismael Miguel 4 hours ago
    
@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. –  Edenia 3 hours ago
    
I think you've picked the wrong supporting argument for why return(0) is bad. Why would anyone mangle code via skype? –  Pimgd 1 hour ago

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.

share|improve this answer
    
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. –  Ismael Miguel 4 hours ago

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);
}
share|improve this answer
    
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. –  Ismael Miguel 4 hours ago
    
@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. –  200_success 4 hours ago
    
@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. –  200_success 4 hours ago
    
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. –  Ismael Miguel 4 hours ago
    
@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. –  200_success 4 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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