Addition or Subtraction Calculator that Processes Up to 15 Operands

Question

(This is homework, however I have a working solution that I'm trying to improve upon)

I'm working on a program that adds or subtracts up to 15 decimal or hexadecimal numbers from the command line. You run the program with the arguments and then choose the option from a menu. I.e.:./main 5 10 15 20 will take in those four arguments and allow you to either add or subtract.

I have a working program, but I am trying to optimize it to reduce some redundancy. Basically my add and subtract functions are identical other than some sign switches, and I feel like there might be a better way. Here is what I have so far (the program can accept decimal input, or hex input formatted with a preceding 0x, which is why there are a sections dealing with those conversions):

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

void menu(char **options){
  for(int i = 0; i < 3; ++i){
    printf("%d. %s\n", i, options[i]);
  }
  printf("Menu Item: ");
}

int isHex(char *argv, int len){
  
  if(argv[0] == '0' && (argv[1] == 'x' || argv[1] == 'X')){
    for (int i = 2; i < len; ++i)
      if(!isxdigit(argv[i]))
        return 0;
    return 1;
  }

  return 0;
}

int convertHex(char *hexVal, int len){

  int dec_val = 0;
  int base = 1;

  for (int i=len-1; i>=2; i--) {    
    if (hexVal[i]>='0' && hexVal[i]<='9'){ 
        dec_val += (hexVal[i] - 48)*base; 
        base = base * 16; 
    } 

    else if (hexVal[i]>='A' && hexVal[i]<='F') { 
        dec_val += (hexVal[i] - 55)*base;
        base = base*16; 
    } 
  }
  
  return dec_val; 
}

void add(int argc, char **argv){
  int i = 1, s = 0, x = 0;

  if(isHex(argv[i], strlen(argv[i])))
    s = (x =convertHex(argv[i], strlen(argv[i])));

  else
    s = atoi(argv[i]); 

  printf("%s + ", argv[i]);

  for(i = 2;i<argc;i++){
    if(isHex(argv[i], strlen(argv[i]))){
      s += (x = convertHex(argv[i], strlen(argv[i])));
      printf("%s", argv[i]);
    }

    else{
      printf("%s", argv[i]);
      s += atoi(argv[i]);
    }

    if(i == argc-1)
      printf(" = ");
    else
      printf(" + ");
   }

   printf("%d\n", s);
}

void sub(int argc, char **argv){

  int i = 1, s = 0, x = 0;

  if(isHex(argv[i], strlen(argv[i])))
    s = (x =convertHex(argv[i], strlen(argv[i])));

  else
    s = atoi(argv[i]); 

  printf("%s - ", argv[i]);

  for(i = 2;i<argc;i++){
    if(isHex(argv[i], strlen(argv[i]))){
      s -= (x = convertHex(argv[i], strlen(argv[i])));
      printf("%s", argv[i]);
    }

    else{
      printf("%s", argv[i]);
      s -= atoi(argv[i]);
    }

    if(i == argc-1)
      printf(" = ");
    else
      printf(" - ");
   }

   printf("%d\n", s);
}

int main(int argc,char** argv){

  char *choices[] = {"Exit", "Addition", "Subtraction"};
  void (*calc[])(int, char**) = {NULL, add, sub};


  if(argc < 3)
    fprintf(stderr, "You must enter at least two arguments.\n");

  else if(argc > 16)
    fprintf(stderr, "You cannot enter more than 15 arguments.\n");

  else{

    int ch=1;
    while(ch!=0){   
      menu(choices);
      scanf("%d",&ch);

      switch(ch){
        case 0:
          exit(0);
          break;

        case 1:
          calc[1](argc,argv);
          break;
        
        case 2:
          calc[2](argc,argv);
          break;
          
        default:
          printf("Enter valid option\n");
          break;
      }
    }

    exit(EXIT_SUCCESS);
  }

  exit(EXIT_FAILURE);
}

The relevant functions are add/sub. It gets tricky because of needing to output the operations to the user. My initial inclination was to go with something like an op-code 1 = add, 2 = sub, but I don't know if that just adds more if-checks in one function and ultimately makes it as long or harder to read.

Example:

void calc(int argc, char **argv, int op){

  int i = 1, s = 0, x = 0;
  char sign = (op == 1) ? '+' :  '-';

  if(isHex(argv[i], strlen(argv[i])))
    s = (x =convertHex(argv[i], strlen(argv[i])));

  else
    s = atoi(argv[i]); 

  for(i = 2;i<argc;i++){

    //Add
    if(op == 1){
      if(isHex(argv[i], strlen(argv[i]))){
        s += (x = convertHex(argv[i], strlen(argv[i])));
        printf("%s", argv[i]);
      }

      else{
        printf("%s", argv[i]);
        s += atoi(argv[i]);
      }

      if(i == argc-1)
        printf(" %c ", sign);
      else
        printf(" %c ", sign);
    }

    //Repeat for subtract

    //Print total
    printf("%d\n", s);
  }
}

But that seems like just making 1 function larger without really optimizing anything.

Does anyone have any suggestions or helpful advice for reducing the redundancy with add/sub (if its even possible)? Its not entirely necessary, but I'd appreciate any assistance.

Answer 1

Here are some things that may help you improve your code.

Eliminate "magic numbers"

Instead of hard-coding the constants such as 3, 48 and 55 in the code, it would be better to use a #define or const and name them. Or, in the case of the convertHex code, use the character constant '0' instead of 48.

Use standard functions where applicable

Instead of this:

if(isHex(argv[i], strlen(argv[i])))
    s = (x =convertHex(argv[i], strlen(argv[i])));
else
    s = atoi(argv[i]); 

I think I'd write this:

s = strtol(argv[i], NULL, 0);

Pass values rather than hardcoding them

I'd suggest changing the signature of the menu function to this:

int menu(size_t numoptions, const char *const *options)

Note that we use const to show that we are not changing the options and that we pass the number of options. Now we can use this with any number of options rather than only three. Also, since the next thing we always do after showing the menu is getting the choice from the user, why not have menu return that choice?

Refactor aggressively

There is not only a lot of repetition here, but also a mixing of tasks. For instance, the add routine converts the numbers, adds them and prints them. Also, the only difference between add and subtract is, of course, the operation applied. I'd separate those like this:

int applyOperator(int argc, char **argv, int (*op)(int, int)) {
    int retval = strtol(argv[0], NULL, 0);
    for (int i = 1; i < argc; ++i) {
        retval = op(retval, strtol(argv[i], NULL, 0));
    }
    return retval;
}

void printList(int argc, char **argv, const char *sep) {
    printf("%s", argv[0]);
    for (int i = 1; i < argc; ++i) {
        printf("%s%s", sep, argv[i]);
    }
}

Now we can create trivially simple add and subtract functions:

int add(int a, int b) {
    return a+b;
}

int subtract(int a, int b) {
    return a-b;
}

And within main, use them like this:

case 1:
  printList(argc, argv, " + ");
  printf(" = %d\n", applyOperator(argc, argv, add));
  break;

Note also that we adjust argc and argv within main so that the arguments passed to the functions start at zero.

Eliminate unused cases

There is no way that case 0 will ever be executed. For that reason, I'd be inclined to omit it.