25
\$\begingroup\$

In a YouTube video I watched, the speaker said it is important to know how to calculate between numbers of different bases; i.e. two, ten, and sixteen. I was then inspired to write a small C program to ensure I understand how such conversions are done. I then wrote a short Python2 script to test the program. I'd like any constructive feedback on the coding style, layout, efficiency, and anything else that stands out. Debug lines are commented out.

calc.c

#include <math.h>
#include <stdio.h>
#include <string.h>

typedef enum {
    AND = '&',
    OR = '|',
    XOR = '^',
    NONE = 0
} operator;

typedef enum {
    B02 = 'b',
    B10 = 'd',
    B16 = 'x'
} type;

const char zero = '0';
const char _A = 'A';
const char _a = 'a';

int main(int argc, char **argv) {
    int i, offs, len;
    char *nend;
    long result = 0, tmp, optmp;
    operator op = NONE;
    type ntype;

    for (i = 1; i < argc; i++) {
        len = strlen(argv[i]);
        nend = argv[i] + len - 1;
        if (len == 1) {
            op = *nend;
            continue;
        }
        ntype = *nend;
        nend--;
        offs = 0;
        optmp = 0;
        while (offs < len - 1) {
            switch (ntype) {
            case B02:
                tmp = (*(nend - offs) - zero) * pow(2, offs);
                break;
            case B10:
                tmp = (*(nend - offs) - zero) * pow(10, offs);
                break;
            case B16:
                tmp = *(nend - offs);
                if (tmp >= _a) tmp -= _a - 10;
                else if (tmp >= _A) tmp -= _A - 10;
                else tmp -= zero;
                tmp *= pow(16, offs);
                break;
            }
            optmp += tmp;
            offs++;

        }
//printf("%s: offs: %i tmp: %li optmp: %li len: %i result: %li\n", argv[i], offs, tmp, optmp, len, result);
        switch (op) {
        case AND:
            result &= optmp;
            break;
        case OR:
            result |= optmp;
            break;
        case XOR:
            result ^= optmp;
            break;
        case NONE:
            result += optmp;
            break;
        }
//printf("\n");
    }
    printf("%li\n", result);
    return 0;
}

makecalc.sh

gcc -Wall calc.c -o calc

testcalc.py

import subprocess as subp
import time

_bin = lambda x: bin(x)[2:] + 'b'
_dec = lambda x: str(x) + 'd'
_hex = lambda x: hex(x)[2:] + 'x'

ops = ('&', '|', '^')
types = (_bin, _dec, _hex)
RANGE = 20

tmp = None

def test(op, t, x, y):
    call = ["./calc", t(x), op, t(y)]
    print call
    proc = subp.Popen(call, stdout=subp.PIPE, stdin=subp.PIPE)
    data = proc.stdout.readline().replace("\n", "")
    exec("tmp = %s %c %s;" % (str(x), op, str(y)))
    if tmp != int(data):
        print ("fail", tmp, data)
        quit()

for op in ops:
    for t in types:
        for x in range(RANGE):
            for y in range(RANGE):
                test(op, t, x, y)
\$\endgroup\$
21
+50
\$\begingroup\$

I suppose you don't indent the printf debug lines to make them easier to spot. That's a sensible idea (although not one I've seen before). However, I treat Code Review answers as a "just before checkin" thing, so I'd suggest removing these by this point.

You pre-declare your variables. Don't do this; write them as close to point-of-use as reasonable.

Your NONE op seems to be rather useless - you could just use OR instead. So remove it.

Your

const char zero = '0';
const char _A = 'A';
const char _a = 'a';

variables might seem like a good adherence to "no magic constants", but in reality they have effectively no useful purpose. If you wanted to update your code to support other character sets, you'd want to redesign the whole parser anyway. IMHO, it's simpler to inline these.

You're not required to return 0; in modern C, and it's somewhat typical not to.

Your integer parser should be generic in the base. I suggest extracting this into a function like int parse_digit(char character, int base, int place):

int parse_digit(char character, int base, int place) {
    int value;

    if (character >= 'a') {
        value = character - 'a' + 10;
    }
    else if (character >= 'A') {
        value = character - 'A' + 10;
    }
    else {
        value = character - '0';
    }

    return value * pow(base, place);
}

while (offs < len - 1) {
    optmp += parse_digit(*(nend - offs), base, offs);
    offs++;
}

base can be calculated with a simple lookup on ntype:

int type_to_base(type ntype) {
    switch (ntype) {
    case B02: return 2;
    case B10: return 10;
    case B16: return 16;
    }
}

Although this makes the type enum rightly look silly: it's just an integer after all!

int flag_to_base(char flag) {
    switch (flag) {
    case 'b': return 2;
    case 'd': return 10;
    case 'x': return 16;
    }
}

Note that this is a somewhat silly way to do things, though. First of all, pow takes doubles! I'd rather avoid that if possible. Further, this is more work than needs to be done. Instead, one can do this:

  1*base^4 + 0*base^3 + 1*base^2 + 0*base^1 + 1*base^0  // how you're currently doing it
= (((1 * base + 0) * base + 1) * base + 0) * base + 1          // simpler way to do it

This can be done as:

int parse_digit(char character) {
    if (character >= 'a') {
        return character - 'a' + 10;
    }
    else if (character >= 'A') {
        return character - 'A' + 10;
    }
    else {
        return character - '0';
    }
}

while (offs < len - 1) {
    optmp *= base;
    optmp += parse_digit(*(nend - offs));
    offs++;
}

Parsing integers should be in another function, and can be slightly simplified:

long parse_int(char *start, char *end) {
    int base = flag_to_base(*end);

    long ret = 0;
    for (char *digit = start; digit < end; digit++) {
        ret *= base;
        ret += parse_digit(*digit);
    }

    return ret;
}

I would also put application of optmp into a function, and stop calling it optmp (in general, most variables are temporary - stating this is pointless):

long apply(operator op, long left, long right) {
    switch (op) {
    case AND: return left & right;
    case OR:  return left | right;
    case XOR: return left ^ right;
    }
}

I would then change (generally) long to uint64_t and int to uint32_t (or even uint8_t) - there's no reason to be using variable-length integers in this day and age for these types, and you don't need signed values. When you do want variable-length values, you'd probably rather stick with size_t. (If you find these unwieldy, typedef them to u64, u32 and u8. Just don't use int and long 'cause they're prettier.)

Compiling with warnings, I get

calc.c: In function ‘apply’:
calc.c:18:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
calc.c: In function ‘flag_to_base’:
calc.c:26:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

This is because the new method allows better case analysis - we've forgotten to check that such operations are actually valid!

#include <math.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>

typedef enum {
    AND = '&',
    OR = '|',
    XOR = '^',
} operator;

int apply(operator op, uint64_t *left, uint64_t right) {
    switch (op) {
    case AND: *left &= right; break;
    case OR:  *left |= right; break;
    case XOR: *left ^= right; break;
    default: return -1;
    }
    return 0;
}

int flag_to_base(char flag, uint8_t *base) {
    switch (flag) {
    case 'b': *base = 2; break;
    case 'd': *base = 10; break;
    case 'x': *base = 16; break;
    default: return -1;
    }
    return 0;
}

int parse_digit(char digit, uint8_t *value) {
    if ('a' <= digit && digit <= 'z') {
        *value = digit - 'a' + 10;
    }
    else if ('A' <= digit && digit <= 'Z') {
        *value = digit - 'A' + 10;
    }
    else if ('0' <= digit && digit <= '9') {
        *value = digit - '0';
    }
    else {
        return -1;
    }

    return 0;
}

int parse_int(uint64_t *ret, char *start, char *end) {
    uint8_t base;
    if (flag_to_base(*end, &base)) {
        return -1;
    }

    *ret = 0;
    for (char *digit = start; digit < end; digit++) {
        uint8_t value;
        if (parse_digit(*digit, &value) || value >= base) {
            return -1;
        }

        *ret *= base;
        *ret += value;
    }

    return 0;
}

int main(int argc, char **argv) {
    uint64_t result = 0;
    operator op = OR;

    for (int i = 1; i < argc; i++) {
        size_t len = strlen(argv[i]);
        char *end = argv[i] + len - 1;

        if (len == 1) {
            op = *end;
            continue;
        }

        uint64_t operand;
        if (parse_int(&operand, argv[i], end)) {
            printf("Can't parse argument '%s'\n", argv[i]);
            return -1;
        }

        if (apply(op, &result, operand)) {
            printf("Can't parse argument '%c'\n", op);
            return -1;
        }
    }

    printf("%li\n", result);
}

Note that I would be wary of casting to enums where this might result in an "invalid" enum, but at least it is well-defined behaviour.


For the Python, your

_bin = lambda x: bin(x)[2:] + 'b'
_dec = lambda x: str(x) + 'd'
_hex = lambda x: hex(x)[2:] + 'x'

can be just

_bin = "{:b}b".format
_dec = "{:d}d".format
_hex = "{:x}x".format

Your embedded loops can be simplified to

from itertools import product
for args in product(ops, types, range(RANGE), range(RANGE)):
    test(*args)

You don't use time - remove it. Rather than rename subprocess to subp, I would directly import Popen and PIPE.

Use new-style formatting, please! Also, use eval instead of exec - using neither would be best, although its use is mildly understandable in this case (even if still saddening).

Don't call quit - this is a function only for interpreter usage. I actually suggest just asserting in this case.

Your subprocess call only wants output, so drop stdin=PIPE. Further, you can just call check_output:

import subprocess
from itertools import product

def test(op, t, x, y):
    call = ["./calc", t(x), op, t(y)]
    print call
    data = subprocess.check_output(call)
    assert eval("{}{}{}".format(x, op, y)) == int(data)

ops = "&|^"
types = "{:b}b".format, "{:d}d".format, "{:x}x".format
count = 20

for args in product(ops, types, range(count), range(count)):
    test(*args)
\$\endgroup\$
  • 1
    \$\begingroup\$ Great answer! Could you elaborate on your point about not using variable length types? Sounds interesting, but nothing I've really seen being done. \$\endgroup\$ – jacwah Jun 14 '15 at 20:48
  • 1
    \$\begingroup\$ @jacwah The sizes of int and long (and thus the values they can hold) are platform-dependent. On the other hand, int32_t is always 32 bits long. Using int just means that your code works differently on different platforms, which is generally not what you want. \$\endgroup\$ – Veedrac Jun 14 '15 at 21:05
  • 1
    \$\begingroup\$ "You pre-declare your variables. Don't do this..." Will you elaborate regarding this preference? \$\endgroup\$ – motoku Jun 14 '15 at 22:45
  • 1
    \$\begingroup\$ Yes. I am asking why it is preferred because my instructors taught me otherwise. \$\endgroup\$ – motoku Jun 14 '15 at 22:57
  • 3
    \$\begingroup\$ @MotokoKusanagi The idea is to minimize scope of variables, which also minimizes the number of live variables at any point. This improves flexibility by decreasing cross-variable interactions and is generally just easier to read. The internet has a few questions on this, too. \$\endgroup\$ – Veedrac Jun 15 '15 at 1:10
18
\$\begingroup\$

This is very nicely written, pleasure to read. Most of my comments will concern usability and readability.

The NONE operator

typedef enum {
    AND = '&',
    OR = '|',
    XOR = '^',
    NONE = 0
} operator;

NONE kinda sticks out there: it's not an operator. It made me wonder how it's used. It turns out, if the operator is NONE, then the calculator performs addition with +.

As such, it would be better to change this to ADD = '+'. That way, everything becomes clear, except the question of why make addition the default, but that's not a big problem.

Input validation

When you assign the operator, you need to validate the input. For example, if you run this:

./calc 5d x 4d 6d

On my computer, I get 5 as output, which doesn't make any sense. What happens, of course, is the default operator is addition, as we've seen earlier, so 5 gets added to result, and after that there will be no further operations because the value of op will be 'x', which won't match any of the case statements.

The user-friendly thing to do would be to print an error and halt execution.

Similarly, the operands are not validated either, which can lead to unexpected results. Again, crashing with an error would be better.

switch statements should have a default case

It's usually recommended that switch statements have a default case. Your two switch statements don't have it, and it makes the reader guess what will happen to the program if none of the cases match.

Naming

I would find this more intuitive than the original, and only slightly longer:

typedef enum {
    BASE_2 = 'b',
    BASE_10 = 'd',
    BASE_16 = 'x'
} type;

Alternatively:

typedef enum {
    BINARY = 'b',
    DECIMAL = 'd',  // or DENARY
    HEXADECIMAL = 'x'
} type;

One of them is not like the others:

const char zero = '0';
const char _A = 'A';
const char _a = 'a';

I'm looking at zero. You could make it look like the others easily:

const char _0 = '0';

Another thing, I sort of skipped over reading the enum elements, and later in the middle of the code when I saw zero, _a, _A, it wasn't really clear what they were. This maybe a matter of taste, but I would actually prefer '0', 'a', 'A' instead of the named constants.

Duplicated logic

This bit appears in each of the case statements:

*(nend - offs)

I would give it a name before the switch, and then reuse it in the cases:

char c = *(nend - offs);

Usability

A minor usability gripe I have about this is that when running the program in the shell, I have to escape some of the operators, for example:

./calc 5d '&' 9d

Not sure what would be a good solution for it though. Just letting you know.

Another thing, I noticed that in the Python code you convert the 0xNUM and 0bNUM notations to your very own NUMx, NUMb notation. How about reusing Python's? At least you'll be following some standard, rather than inventing something new. The good thing about using something existing that it's familiar to users. The first thing I did was I tested your code without reading it, and I was a bit frustrated when ./calc 5 '|' 9 didn't give what I was expecting.

Python 2?

Why Python 2? Why not Python 3?

Btw, one of your print statements uses parentheses (and thus ready for Python 3), while the other doesn't. It would be good to make both ready for Python 3.

\$\endgroup\$
  • \$\begingroup\$ The NONE "operator" is there to allow for the placement of the first number within the sequence of arguments. Thanks for this review. \$\endgroup\$ – motoku Jun 14 '15 at 18:47
  • \$\begingroup\$ Most of the issues you pointed out here I had thought about; but not to such detail. My thinking was I didn't want to clutter up the code; but you made the necessity for validity checks apparent. I'm also thinking of using strcmp with the operators, e.g. AND OR XOR (no more escape characters). \$\endgroup\$ – motoku Jun 14 '15 at 19:08
3
\$\begingroup\$

I would definitely break the main function into several smaller ones, and find the best possible names for these. int convert(const char* str, type in_type), bin2hex(...), hex2bin(...), dec2bin(...), int operator_apply(int a, int b, operator op).

This would make main() much easier to overview, and it would be self-documenting (with good identifiers) what different steps do. Right now all the argument parsing, conversions, operations, are intertwined and hard to read.

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