2
\$\begingroup\$

This is a simple exercise in C designed to work on IO. The purpose of the program is simple: convert a number of hours and minutes to a total number of minutes.

#include <stdio.h>

const unsigned short MinutesPerHour = 60;

int main(int argc, char* argv[])
{
    int hours, minutes, total;

    if(argc == 1)
    {
        puts("Enter the number of hours:");
        scanf("%d", &hours);      
        puts("Enter the number of minutes:");
        scanf("%d", &minutes);
    }
    else if(argc == 2)
    {
        hours = atoi(argv[1]);
        minutes = 0;
    }
    else if(argc == 3)
    {
        hours = atoi(argv[1]);
        minutes = atoi(argv[2]);
    }
    else if(argc != 3)
    {
        printf("Usage: %s (int hours) (int minutes)\n", argv[0]);

        return -1;
    }

    total = (hours * MinutesPerHour) + minutes;  
    printf("%d hourss and %d minutes is %d minutes\n", hours, minutes, total);

    return 0;
}

Is there specifically any way I could improve the argument/input parsing blocks? Would it be more consistent to ask the user for minutes if they supply an hours command line argument? Should I offer the variety of options at all, or should I stick to command line arguments only or "dynamic" input only?

Bonus: Are there any meaningful names that could be assigned to the 1, 2, and 3 in the parsing blocks that would make the code more readable? Magic numbers are bad, but since it is checking the number of arguments, I can't think of anything other than if (argc == OneArgument) else if (argc == TwoArguments)... which seems silly.

\$\endgroup\$
  • \$\begingroup\$ Do you want to accept any amount of minutes, or just the minutes that are part of an hour? Also do you want to accept hours greater than 23? It seems for me your IO requirements aren't clearly specified enough. \$\endgroup\$ – πάντα ῥεῖ Jan 17 '17 at 21:32
5
\$\begingroup\$

OP's Questions

  1. Is there specifically any way I could improve the argument/input parsing blocks?
    See #3 far below

  2. Would it be more consistent to ask the user for minutes if they supply an hours command line argument?
    Could go either way on that. #3 far below does that.

  3. Should I offer the variety of options at all, or should I stick to command line arguments only or "dynamic" input only?

Break the needed input into 2 groups: required and optional. An optional parameter may be the format to print the result another way - it is not required to run the core function of code. A required one is the hours. Minutes could go in either group.

If none of the required parameters are supplied, use interactive mode.

If all of the required parameters are supplied, proceed without asking.

If partial required parameters are supplied, error out.

  1. Are there any meaningful names that could be assigned to the 1, 2, and 3 in the parsing blocks that would make the code more readable?

Not really. Alternatively the argument should be paired with a prefix one. Sample code invocation:

    MBM_Minutes -hour 12 -minute 33
    same result as 
    MBM_Minutes -minute 33 -hour 12

Optional arguments could be "-help" "-format AMPM", etc.

Other observations

argv[]

  1. Should code trust that the string will convert to a valid int number of hours? Look to strtol() or sscanf() if in doubt (or experienced).

    // hours = atoi(argv[1]);
    
    if (sscanf(argv[1], "%d", &hours) == 1) Success();
    else Handle_Failure();
    
    // better strtol() example down below   
    
  2. The last should be a catch-all else.

    // else if(argc != 3) {
    else {
      printf("Usage: %s (int hours) (int minutes)\n", argv[0]);
    
  3. From a code layout, often is is best to qualify variables first, then proceed to the normal flow of code. Note: argc == 0 is possible, though rare.

    if (argc < 1 || argc > 3) {
      printf("Usage: %s (int hours) (int minutes)\n", argv[0]);
      // or send to stderr
      fprintf(stderrr, "Usage: %s (int hours) (int minutes)\n", argv[0]);
      return -1;
    }
    
    int hours;
    if (argc < 2) {
      GetInt("Enter hours:", &hours, 0, 24);
    } else {
      hours = atoi(argv[1]);
    }
    
    int minutes;
    if (argc < 3) {
      GetInt("Enter minutes:", &hours, 0, 59);
    } else {
      hours = atoi(argv[1]);
    }
    

User Input

  1. When dealing with user input, avoid being a hacker target. Step 1: check user input function results.

    puts("Enter the number of hours:");
    if (scanf("%d", &hours) != 1) {
      perror("Number not entered\n");
      eixt(-1);
    }
    
  2. To be certain the output is truly outputted and not buffered, make certain the output is flushed before attempting input.

    puts("Enter the number of hours:");
    fflush(stdout);
    if (scanf("%d", &hours) != 1) {
    
  3. Input values should be qualified to the accept range.

    if (scanf("%d", &hours) != 1) {
      ...
    }
    if (hours < 0 || hours > 24) {
      Handle_Range_Error(hours, 0, 24);
      ...
    }
    
  4. Take user input in a string and then parse it. Easier to cope with bad input. Suggests avoiding scanf() and use fgets().

  5. Common code should exist in a helper function. Putting the above ideas together results in a not-so-simple code and it is certainly overkill for a simple exercise. Yet it is here to give you ideas of what it takes to handle general user input.

    // return 0 on success 
    int GetInt(const char *prompt, int *dest, int min, int max) {
      long number;
      do {
        fputs(prompt);
        fflush(stdout); 
        char buf[100];
        if (fgets(buf, sizeof buf, stdin) == NULL) return EOF;
        char *endptr;
        errno = 0;
        long number = strtol(buf, &endptr, 10);
        if (endptr == buf) continue;  // no conversion
        if (errno) continue; //overflow in conversion
        if (number < min || number > max) continue; // out of range
        while (isspace(*endptr)) endptr++;
      } while (*endptr); // extra junk at the end of the string
      *dest = (int) number;
      return 0;
    }
    
    int main(...
    
       if (GetInt("Enter hours:", &hours, 0, 24)) {
         Handle_EOF();  // Maybe post an error message and exit
       }
    

Constants

Alternatives to declaring a global object. There are various pros and cons to these. What is best is to code to your group's coding standards. In doubt, go with the last, simplest one.

    const unsigned short MinutesPerHour = 60;
    // or full width to avoid promote to `int`
    const unsigned MinutesPerHour = 60;
    // or
    #define MinutesPerHour 60
    // or insure unsigned-ness
    #define MINUTES_PER_HOUR 60u
    // or classically 
    #define MINUTES_PER_HOUR 60

Minor

Spelling: hourss --> hours

\$\endgroup\$
  • \$\begingroup\$ I think return EXIT_FAILURE; is better than return -1; and eixt(-1);; you'll need to include <stdlib.h> for the definition. In any case, prefer to be consistent (always return or always exit()). Also, what do you mean by "Common code should exit in a helper function"? Don't exit unless you really can't return. \$\endgroup\$ – Toby Speight Jan 18 '17 at 12:07
  • \$\begingroup\$ @TobySpeight Agreed. "exit" was a typo. Corrected to "exist". \$\endgroup\$ – chux Jan 18 '17 at 15:01
  • \$\begingroup\$ Ah, "exist" - I should have worked that out! \$\endgroup\$ – Toby Speight Jan 18 '17 at 19:00
1
\$\begingroup\$

the function: atoi() requires the statement: #include <stdlib.h> // atoi()

however, that function is frowned upon in favor of strtol() so you might consider using that function instead

\$\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.