# Addition or Subtraction Calculator that Processes Up to 15 Operands

(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>

for(int i = 0; i < 3; ++i){
printf("%d. %s\n", i, options[i]);
}
}

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;
}

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){
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++){

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.

• Please edit the title to describe what the code does rather than what you want from a review. E.g. "Calculator that handles up to fifteen operands". Commented Nov 10, 2020 at 3:29
• Please do not edit the question after an answer has been posted, especially the code. Commented Nov 10, 2020 at 18:33
• Ah ok, gotcha. Still learning protocols here. I didn't edit the original code, just posted the "after", so now it has a picture of both before and after. Commented Nov 10, 2020 at 18:34

## 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

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.

• Thank you, I will work on implementing some of these changes and see how it goes. Commented Nov 10, 2020 at 16:16
• with your section "Use Standard Functions", I had read up on strtol and never realized that using the special value 0 would detect the 0x ahead of a hex. This was extremely helpful. Commented Nov 10, 2020 at 17:27