0

I wrote a simple program to find greatest among three numbers. But it seems that I wrote it in a way that makes it slightly confusing - hard to understand. What would be the way to improve this program to make it better at expressing its purpose and operation, and to remove the obvious repetition?

main() 
{
    int a,b,c;
    printf("Enter three numbers: ");
    scanf("%d %d %d",&a,&b,&c);
    if (a==b && b==c)     
        printf("all are equal....:)");
    else if(a>b)
        {    
             if(a>c)
                  printf("%d is greatest among all",a);
             else  
                 printf("%d is greatest among all",c);
        }
        else
        {
             if(b>c)
                 printf("%d is the greatest among all",b);
             else
                 printf("%d is the greatest among all",c);
        }
    getch();
}
Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • 1
    Instead of duplicating the `printf("%d is the greatest among all", variable);` I suggest to assign the greatest value to a variable in all branches and print it afterwards. Something like `int a, b, c, greatest;` ... `else if(a>b) { if(a>c) greatest = a; else greatest = c }` ... `printf("%d is the greatest among all", greatest);` – Bodo Mar 12 '20 at 15:44
  • 1
    `scanf("%d%d%d", ...)` ... `"%d"` means "ignore optional whitespace and read an integer", `" %d"` means "ignore optional whitespace **twice** and read an integer" – pmg Mar 12 '20 at 16:14
  • 1
    I'm voting to close this question as off-topic because it is an open-end request for improvement of working code. Try codereview.stackexchange.com – chepner Mar 12 '20 at 21:56

3 Answers3

1
#include <stdio.h>

int max(int a, int b)
{
    return (a > b) ? a : b;
}

int main(void)
{
    int a, b, c;
    printf("Enter three number: ");
    scanf("%d %d %d", &a, &b, &c);

    if ((a == b) && (b == c))
    {
        printf("all are equal.");
    }
    else
    {
        printf("%d is the greatest among all", max(a, max(b, c)));
    }

    return 0;
}
0

I'd like to offer an example of the development process that could lead to figuring it out. I've linked relevant documentation to fill in the likely gaps in knowledge, and attempted to explain what led me to that documentation. Variants of such thought process are common among developers, yet the process is regrettably often not explained in educational settings, and must be acquired by "sweat and tears". I figure: let's shed some light on it!

A decent reference to the C language and standard library is https://en.cppreference.com/w/c (yes, it's cppreference, but it's not just for C++!).


The clarity of this program matters, and it certainly could be improved.

But people also interpret efficiency to mean speed. But: The speed of this particular program is irrelevant. The slowest element is the human, by more than 6 orders of magnitude vs. the machine. If you wished for this code to be faster when e.g. working on a large array of triples of numbers, then you could edit the question and instead provide an example that works through a big array of triples. As it stands, the program could be written in a shell script file and you wouldn't be able to tell the difference. It's a good idea to optimize what matters - and maintainability often matters more than raw performance :)

Output Skeleton

You only need to print the largest number - so let's start with a way to print it, and assume that the selection of the largest number, and the detection whether they are all equal, has been taken care of:

#include <stdio.h>

int main() {
    char all_equal;
    int max;
    // to be done

    if (all_equal) printf("All numbers are equal.\n");
    else printf("%d is the greatest among the numbers entered.\n", max);
}

Thus, we have a skeleton for the output of the program. That's our goal. It helps to start with the goal expressed in the language of the problem domain - here in C - for it can focus and guide the implementation. We know exactly where we're going now: we have to get the input, and then process it to obtain the max value, and the all_equal predicate (a boolean: zero means falseness, anything else means truth).

We could actually turn this goal into a functional program by providing some "fake" data. Such data could be called test data, or mock data, depending on who you ask :)

#include <stdio.h>

int main() {
    char all_equal = 0;
    int max = 10;

    if (all_equal) printf("All numbers are equal to %d.\n", max);
    else printf("%d is the greatest among the numbers entered.\n", max);
}

We can run this, then change all_equal to 1 and see how the output is changed. So - we have some idea about what results should be fed into the output section of the program.

Processing

Getting the input from the user is on the opposite end of the goal, so let's work on something that builds up directly on the goal: let's compute those all_equal and max values! The processing code should replace the mock char all_equal = 0; int max = 10; section.

First, we need some numbers - we'll use mock data again, and from them we need to select the largest number:

int numbers[3] = {-50, 1, 30}; // mock data

char all_equal = 0; // mock data
int max = numbers[0];
for (int i = 1; i < 3; i++) {
  if (numbers[i] > max) max = numbers[i];
}

Modern compilers will usually unroll this loop, and the resulting code will be very compact. The i loop variable won't even appear in the output, I'd expect.

We can foresee a potential for mistakes: if we ever decide to change the quantity of numbers, we can easily miss a place where such quantity is hardcoded (here as the literal 3). It'd be good to factor it out:

#define N 3
int numbers[N] = {-50, 1, 30}; // mock data

char all_equal = 0; // mock data
int max = numbers[0];
for (int i = 1; i < N; i++) {
  if (numbers[i] > max) max = numbers[i];
}

You'll run into such #define-heavy code. It has its place back when C compilers were poor at optimizing code. Those times are now thankfully well over, but people continue doing this - without quite understanding why it is that they do it.

For numeric constants, it's usually unnecessary and counterproductive: the preprocessor does string substitution, so it has no idea what the code it modifies actually means. Preprocessor definitions "pollute" the global context - they leak out from wherever we define them, unless we explicitly #undef-ine them (How many people do that? You'll find that not many do.)

Instead, let's express it as a constant. We try:

const int N = 3;
int numbers[N];

But: this doesn't compile on some compilers. Huh? We read up on array declarations, and it seems that iff variable-length arrays (VLAs) are not supported by our compiler, then the number of elements must be a [constant expression][constexpr]. C doesn't consider a const int to be a constant expression (how silly, I know!). We have to kludge a bit, and use an enumeration constant to get the constant expression we need:

enum { N = 3 };
int numbers[N] = {-50, 1, 30}; // mock data

char all_equal = 0; // mock data
int max = numbers[0];
for (int i = 1; i < n; i++) {
  if (numbers[i] > max) max = numbers[i];
}

We start by assigning the value of the first number in the list (index 0!) to max. Then we loop over subsequent numbers (starting with index 1!), compare each to the maximum, and update the maximum if a number happens to be greater than the maximum.

That's half of the goal: we got max, but we also need all_equal! The check for quality can be done in the same loop as the selection of the maximum (greatest number), thus:

enum { N = 3 };
int numbers[N] = {-50, 1, 30}; // mock data

char all_equal = 1;
int max = numbers[0];
for (int i = 1; i < N; i++) {
  all_equal = all_equal && numbers[i] == max;
  if (numbers[i] > max) max = numbers[i];
}
// here we have valid all_equal and max

We start with the assumption that the numbers are all equal (all_equal = 1). Then, for each subsequent number (indices 1 onwards), we check if the number is equal to the maximum, and we use a logical conjunction (logical and - &&) to update the all_equal proposition. Proposition is what we call a boolean: simply a statement that can be either true (here: non-zero) of false (zero). The conjunction applied repeatedly to all_equal has the effect of a one-way valve: once all_equal goes false, it will stay false.

Logicians would state it as ∀ p ∈ ℙ : (false ∧ p) = false. Read: for all ℙropositions p, false and p is false.

We merge this into our skeleton, and get a slightly more useful program. It still uses mock data, but it performs all the "interesting" parts of the problem.

#include <stdio.h>

int main() {
  enum { N = 3 };
  int numbers[N] = {-50, 1, 30}; // mock data

  char all_equal = 1;
  int max = numbers[0];
  for (int i = 1; i < N; i++) {
    all_equal = all_equal && numbers[i] == max;
    if (numbers[i] > max) max = numbers[i];
  }

  if (all_equal) printf("All numbers are equal.\n");
  else printf("%d is the greatest among the numbers entered.\n", max);
}

We can run this code, tweak the mock data, and verify that it appears to do what we want it to! Yay!

Input

Finally, we should get those numbers from the user, to get rid of the final bit of mock data:

#include <stdio.h>

int main() {
  enum { N = 3 };
  int numbers[N];
  printf("Enter %d numbers:\n", N);
  for (int i = 0; i < N; i++) scanf("%d", &numbers[i]);

  char all_equal = 1;
  int max = numbers[0];
  for (int i = 1; i < N; i++) {
    all_equal = all_equal && numbers[i] == max;
    if (numbers[i] > max) max = numbers[i];
  }

  if (all_equal) printf("All numbers are equal.\n");
  else printf("%d is the greatest among the numbers entered.\n", max);
}

We run it, and hey! It seems to work! This program is equivalent to the one you wrote. But. There's always a "but".

Handling Invalid Input

Software testing now begins in earnest. We "play" some more with the program (we "exercise" it), and notice that the program doesn't react any differently when a non-number is entered. It sometimes seems to ignore the invalid number and still provide sensible result based on other, correct, numbers, but sometimes it just spits out a random value. Random value - hmm. Could perhaps one of the numbers be uninitialized?

We read the documentation of scanf() and notice that it won't modify its output argument if it fails! Thus the program has undefined behavior: here it results in "random" output.

Reading on, we find that the return value of scanf() can be used to check if it was successful. We'd like to handle invalid input and provide some feedback:

int main() {
  enum { N = 3 };
  int numbers[N];
  printf("Enter %d numbers.\n", N);
  for (int i = 0; i < N; /*empty!*/) {
    printf("#%d: ", i+1);
    if (scanf("%d", &numbers[i]) != 1)
      printf("Sorry, invalid input. Try again.\n");
    else
      i++;
  }
  ...
}

We [try again][run4], and the program goes into an infinite loop as soon as invalid input is provided. Hmm. Something weird is going on. We read up on the issue of parsing user input in C, and realize that scanf() alone won't work correctly if any input errors are present, since the input remains in the input buffer - it will keep "tripping" subsequent scanf's. We must remove that input before retrying.

To find a function that may do that, we read up on the C input/output library and find the getchar() function. We use it:

int main() {
  enum { N = 3 };
  int numbers[N];
  printf("Enter %d numbers.\n", N);
  for (int i = 0; i < N; /*empty*/) {
    printf("#%d: ", i+1);
    if (scanf("%d", &numbers[i]) != 1) {
      printf("Sorry, invalid input. Try again.\n");
      char c;
      while ((c = getchar()) != '\n'); // remove input until end of line
    } else
      i++;
  }
  ...
}

We try it again, and things seem to work. But! We can do something else: let's try closing the input stream (^Z,Enter on Windows, ^D on Unix). The program goes into an infinite loop - again.

Aha! End of input - EOF! We must explicitly handle the EOF (end of file/read error) condition, and the best we can do then is to stop the program. How? We read up about C program utilities, and find a perfect function that would abort the program: abort(). Program utilities are utilities that used to manage "other" tasks that a program might do - tasks that don't fall under other categories (are not I/O, math, etc.).

#include <stdlib.h> // added for abort()

int main() {
  enum { N = 3 };
  int numbers[N];
  printf("Enter %d numbers.\n", N);
  for (int i = 0; i < N; /*empty*/) {
    int c;
    printf("#%d: ", i);
    c = scanf("%d", &numbers[i]);
    if (c == EOF) abort();
    if (c != 1) {
      printf("Sorry, invalid input. Try again.\n");
      while ((c = getchar()) != EOF && c != '\n');
      if (c == EOF) abort();
    } else
      i++;
  }
  ...
}

We try yet again, and this time things really seem to work fine. No matter what we throw at the program, it behaves reasonably: if the input is still available, it asks the user to enter the number again. If the input won't be available anymore (EOF indicates that), we abort().

In even more deviousness, we try to surround the numbers with spaces and tabs in our input - after all, it's just whitespace, and to a human it'd look like valid input in spite of the spaces. We try it, and no problem: scanf() seems to do the "right thing". Hah. But why? We read into the scanf() documentation, and discover that [emphasis mine]:

All conversion specifiers other than [, c, and n consume and discard all leading whitespace characters (determined as if by calling isspace) before attempting to parse the input. These consumed characters do not count towards the specified maximum field width.

Complete Program

We now got all the pieces. Put them together, and the complete program is:

#include <stdio.h>
#include <stdlib.h>

int main() {
  enum { N = 3 };
  int numbers[N];
  printf("Enter %d numbers.\n", N);
  for (int i = 0; i < N; /*empty*/) {
    printf("#%d: ", i+1);
    int c = scanf("%d", &numbers[i]);
    if (c == EOF) abort();
    if (c != 1) {
      printf("Sorry, invalid input. Try again.\n");
      while ((c = getchar()) != EOF && c != '\n');
      if (c == EOF) abort();
    } else
      i++;
  }

  char all_equal = 1;
  int max = numbers[0];
  for (int i = 1; i < N; i++) {
    all_equal = all_equal && numbers[i] == max;
    if (numbers[i] > max) max = numbers[i];
  }

  if (all_equal) printf("All numbers are equal.\n");
  else printf("%d is the greatest among the numbers entered.\n", max);
}

Try it out!.

Useful Refactoring

This works fine, but the input processing dominates over main, and seems to obscure the data processing and output. Let's factor out the input, to clearly expose the part of the program that does "real work".

We decide to factor out the following function:

void read_numbers(int *dst, int count);

This function would read a given count of numbers into the dst array. We think a bit about the function and decide that a zero or negative count doesn't make sense: why would someone call read_numbers if they didn't want to get any input?

We read up on error handling in C, and discover that assert() seems a good candidate to make sure that the function was not called with incorrect parameters, due to a programming bug. Note that assert() must not be used to detect invalid program input!!. It is only meant to aid in finding program bugs, i.e. the mistakes of the developer of the program, not mistakes of the user. If user input had to be checked, it has to be done explicitly using e.g. the conditional statement (if), or a custom function that checks the input - but never assert!

Note how assert is used:

  1. It tells you, the human reader of the program, that at the given point in the program, count must be greater than zero. It helps reasoning about the subsequent code.

  2. The compiler generates the code that checks the asserted condition, and aborts if it doesn't hold (is false). The checks usually are only performed in the debug build of the program, and are not performed in the release version.

To keep the program flow clear, we forward-declare read_numbers(), use it in main(), and define (implement) the function last, so that it doesn't obscure things:

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>

void read_numbers(int *dst, int count);

int main() {
  enum { N = 3 };
  int numbers[N];

  read_numbers(numbers, N);

  char all_equal = 1;
  int max = numbers[0];
  for (int i = 1; i < N; i++) {
    all_equal = all_equal && numbers[i] == max;
    if (numbers[i] > max) max = numbers[i];
  }

  if (all_equal) printf("All numbers are equal.\n");
  else printf("%d is the greatest among the numbers entered.\n", max);
}

void read_numbers(int *dst, int count) {
  assert(count > 0);
  printf("Enter %d numbers.\n", count);
  for (int i = 0; i < count; /*empty*/) {
    printf("#%d: ", i+1);
    int c = scanf("%d", &dst[i]);
    if (c == EOF) abort();
    if (c != 1) {
      printf("Sorry, invalid input. Try again.\n");
      while ((c = getchar()) != EOF && c != '\n');
      if (c == EOF) abort();
    } else
      i++;
  }
}

You can try this program out in onlinegdb.com - just click this link!

In my opinion, main() looks very clear now. The input function stands on its own and can be analyzed in isolation: note how it's a pure function, and has no global state. Its output only acts on the arguments passed in. There are no global variables! There shouldn't ever be, really.

That's where I'd stop. We have a clear program that handles both valid and invalid input.

Moar Refactoring !!111!!

But you could say: how about we factor out data processing and final output as well? We can do that, sure. Yet, in a simple program like ours, it perhaps will slightly obscure what's going on. But at least let's see how it could look then, play with it and decide for ourselves :)

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>

typedef struct {
    char all_equal; // true if all numbers were equal
    int max; // the greatest number
} Result;

void read_numbers(int *dst, int count);
Result find_greatest(int *numbers, int count);
void output_result(const Result *r);

int main() {
  enum { N = 3 };
  int numbers[N];

  read_numbers(numbers, N);
  const Result r = find_greatest(numbers, N);
  output_result(&r);
}

void read_numbers(int *dst, int count) {
  assert(count > 0);
  printf("Enter %d numbers.\n", count);
  for (int i = 0; i < count; /*empty*/) {
    printf("#%d: ", i+1);
    int c = scanf("%d", &dst[i]);
    if (c == EOF) abort();
    if (c != 1) {
      printf("Sorry, invalid input. Try again.\n");
      while ((c = getchar()) != EOF && c != '\n');
      if (c == EOF) abort();
    } else
      i++;
  }
}

Result find_greatest(int *numbers, int count) {
  assert(count > 0);
  Result r = {.all_equal = 1, .max = numbers[0]};
  for (int i = 1; i < count; i++) {
    r.all_equal = r.all_equal && numbers[i] == r.max;
    if (numbers[i] > r.max) r.max = numbers[i];
  }
  return r;
}

void output_result(const Result *r) {
  if (r->all_equal) printf("All numbers are equal.\n");
  else printf("%d is the greatest among the numbers entered.\n", r->max);
}

Note how the Result local variable is initialized using struct initialization with designated initializers:

Result r = {.all_equal = 1, .max = numbers[0]};

If you needed to perform the initialization independently of the declaration of the variable, you would wish to use compound literals - a lesser known, but very important notational shortcut in modern C:

Result r;
// some intervening code
r = (Result){.all_equal = 1, .max = numbers[0]};

or perhaps:

void function(Result r);

void example(void) {
  function((Result){.all_equal = 1, .max = numbers[0]});
}
Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • One returns also the number of maximum values for 3, but not for 2. This is confusing. – Neil Mar 12 '20 at 18:54
  • @Neil Sorry, I have no idea what you tried to say, other than that I do agree with the 2nd sentence (it is indeed totally confusing to me) :( – Kuba hasn't forgotten Monica Mar 12 '20 at 21:54
  • If there's one or two maximums, one just prints the maximal value, if there's three, it prints out that the three are equal; answering this question has made me see that is the original behaviour, as well, so perhaps the OP meant for that to happen. – Neil Mar 13 '20 at 17:01
  • 1
    The cardinality of the set-of-maximums is 0 or 1 under strict total order :) I believe that the OP wanted to indicate that either the values are equal, or that they are unequal with a maximum. So, the output assumes a strict total order (`<` rather than `≤`): only if the values are unequal there can be a maximum. The "all values are equal" output means "no strict total ordering of the numbers exists hence no maximum exists". **The implementation**, on the other hand, uses the variable `max` assuming a non-strict total order, i.e. where `a=b → a≤b` but `a≤b → (a=b ∨ a – Kuba hasn't forgotten Monica Mar 13 '20 at 19:33
  • Yes; _eg_ `{2,2,1}` prints 2 and `{1,1,1}` prints all equal, it can be viewed as a mixing of non-strict / strict. – Neil Mar 13 '20 at 20:29
0

Your code is fine; in fact, Bentley, McIlroy, Engineering a sort function, 1993, realised by, among others, BSD qsort, employs the same sort of mechanism in the inner loop for finding the median of three values. Except,

  1. If all values are the same, it doesn't, "find the greatest among three numbers," but rather short-circuits and finds how many times the maximal value occurs, (three.) This is a separate problem.
  2. It generally pays to separate one's logic from the output.

I've taken your code and put it in a function, stripped the output, and stripped of the equality.

#include <assert.h>

static int hi3(const int a, const int b, const int c) {
    /* Same as `return a > b ? a > c ? a : c : b > c ? b : c;`. */
    if(a > b) {
        if(a > c) {
            return a;
        } else {
            return c;
        }
    } else {
        if(b > c) {
            return b;
        } else {
            return c;
        }
    }
}

int main(void) {

    /* Permutations of 3 distinct values. */
    assert(hi3(1, 2, 3) == 3
        && hi3(1, 3, 2) == 3
        && hi3(2, 1, 3) == 3
        && hi3(2, 3, 1) == 3
        && hi3(3, 1, 2) == 3
        && hi3(3, 2, 1) == 3);

    /* Permutation of 2 distinct values. */
    assert(hi3(1, 2, 2) == 2
        && hi3(2, 1, 2) == 2
        && hi3(2, 2, 1) == 2);
    assert(hi3(1, 1, 2) == 2
        && hi3(1, 2, 1) == 2
        && hi3(2, 1, 1) == 2);

    /* All the same value. */
    assert(hi3(1, 1, 1) == 1);

    return 0;
}

Your code tests all the combinations of relative magnitudes successfully.

Neil
  • 1,767
  • 2
  • 16
  • 22