2
\$\begingroup\$

I have written a python 2 program for a school project to convert from hexadecimal to decimal without using built-in functions and vice versa in python. I would like to know how I can make my code more concise - specifically, the "d_func" function.

d_dict = {"0" : 0, "1" : 1, "2" : 2, "3" : 3, "4" : 4, "5" : 5, "6" : 6, "7" : 7, "8" : 8, "9" : 9, "A" : 10, "B" : 11, "C" : 12, "D" : 13, "E" : 14, "F": 15}

def d_func(digit, mode):
    if mode == 1:
        for x in range(len(d_dict.keys())):
            if digit == list(d_dict.keys())[x]:
                return x
    else:
        for y in range(len(d_dict.values())):
            if digit == list(d_dict.values())[y]:
                return list(d_dict.keys())[y]

def hd_func(h_num):
    d_num, p = 0, 0

    for digit in range(len(h_num), 0, -1):
        d_num = d_num + 16 ** p * d_func(h_num[digit - 1], 1)
        p += 1

    return str(d_num)

def dh_func(d_num):
    f_list, h_num, p = [], "", 0

    while d_num > 0:
        f_list.append(d_num % 16)
        d_num //= 16 

    for f in f_list[::-1]:
        h_num += d_func(f, 2)

    return h_num

func_dict = {"h": hd_func, "d": dh_func}
u_choice, u_num = input("Enter [H] for Hexadecimal to Decimal, [D] for Decimal to Hexadecimal"), input("Enter the number: ")
print(func_dict[u_choice.lower()](u_num))
\$\endgroup\$
  • \$\begingroup\$ Please use python-3.x in the future, as python-2.x has reached it's end of life. \$\endgroup\$ – Linny Jan 17 at 22:04
  • \$\begingroup\$ I use python 3 outside of school but my school uses an online version of python 2, so I am required to write and submit programs in python 2. \$\endgroup\$ – Mattthecommie Jan 17 at 22:07
  • \$\begingroup\$ As long as you know to use python-3.x. :) \$\endgroup\$ – Linny Jan 17 at 22:07
4
\$\begingroup\$

First -- document your code! I read through your d_func function and here's my attempt at writing a docstring for what it does, with Python-2-compatible type hints. Hopefully I got it right. :)

def d_func(digit, mode):
    # type: (Union[str, int], int) -> Union[int, str, None]
    """Mode 1: give the int value of the hex digit.
    Other modes: give the hex digit of the given value.
    Returns None if you give it a bad value, I guess?
    """
    if mode == 1:
        for x in range(len(d_dict.keys())):
            if digit == list(d_dict.keys())[x]:
                return x
    else:
        for y in range(len(d_dict.values())):
            if digit == list(d_dict.values())[y]:
                return list(d_dict.keys())[y]
    return None  # stumblebum

Right off the bat: this should not be one function. (The fact that it can return either an int or a str depending on its input type is a good clue!) You should have one function that converts value to digit and another that converts digit to value; there's no value at all to having one function that does completely different things depending on a flag you pass it.

Second: yes, this can be a lot simpler. I think you could implement d_func as follows:

d_dict = {"0" : 0, "1" : 1, "2" : 2, "3" : 3, "4" : 4, "5" : 5, "6" : 6, "7" : 7, "8" : 8, "9" : 9, "A" : 10, "B" : 11, "C" : 12, "D" : 13, "E" : 14, "F": 15}
d_dict_reverse = {v: d for d, v in d_dict.iteritems()}  # this is just building a new dictionary with the keys and values swapped

def d_func(digit, mode):
    # type: (Union[str, int], int) -> Union[int, str]
    if mode == 1:
        return d_dict[digit]
    else:
        return d_dict_reverse[digit]

At that point, it doesn't need to be a function at all, because you're just doing a simple dictionary lookup. Give your dictionaries reasonable names that say what they do:

digit_to_value = {
    "0" : 0, "1" : 1, "2" : 2, "3" : 3, "4" : 4, "5" : 5, "6" : 6, "7" : 7, 
    "8" : 8, "9" : 9, "A" : 10, "B" : 11, "C" : 12, "D" : 13, "E" : 14, "F": 15
}
value_to_digit = {v: d for d, v in digit_to_value.iteritems()}

and then instead of:

d_num = d_num + 16 ** p * d_func(h_num[digit - 1], 1)

do:

d_num = d_num + 16 ** p * digit_to_value[h_num[digit - 1]]

and instead of:

    for f in f_list[::-1]:
        h_num += d_func(f, 2)

do:

    for f in f_list[::-1]:
        h_num += value_to_digit[f]

With this approach, not only do you not have to write a function, but unlike your function the dictionary will automatically raise a KeyError if you provide the wrong kind of input, e.g. if you do value_to_digit[100] or digit_to_value[1]. Raising an error ASAP when you mess up (aka "fail fast") is good, because it makes it easier to figure out exactly where your bug is. :)

\$\endgroup\$
  • \$\begingroup\$ d_func could be written in one line: return d_dict[digit] if mode else d_dict_reverse[digit] \$\endgroup\$ – Linny Jan 17 at 22:06
2
\$\begingroup\$

In addition to Sam I want to point out some other things

Avoid typing long list/dict constants

Very often you can construct them by code, which is less error prone. Instead of

d_dict = {"0" : 0, "1" : 1, "2" : 2, "3" : 3, "4" : 4, "5" : 5, "6" : 6, "7" : 7, "8" : 8, "9" : 9, "A" : 10, "B" : 11, "C" : 12, "D" : 13, "E" : 14, "F": 15}

you do

import string

digit = dict(zip(range(16), string.digits + string.ascii_uppercase))
value = {v: k for k, v in digit.items()}

If you type all the values you have to write test cases for all of them.

Loop like a pro

You prefer to loop like

for i in range(len(something)):
    print(something[i])

That is not how it is done in Python as it is error prone. In Python you loop like

for e in something:
    print(e)

If for some reason you really also need the index you use enumerate()

for i, e in enumerate(something):
    print(i, e)

That said we change

def hd_func(h_num):
    d_num, p = 0, 0

    for digit in range(len(h_num), 0, -1):
        d_num = d_num + 16 ** p * d_func(h_num[digit - 1], 1)
        p += 1

    return str(d_num)

to

def to_int(s):
     i = 0
     for c in s:
         i = i*16 + value[c]
     return i

The loop is much cleaner. By changing the algorithm I also got rid of the counter. Also I think returning a string is wrong here and I changed that to an int. Also I changed the function name to fit the return type and be less cryptic.

Do not initialize as a tuple if it isn't one

f_list, h_num, p = [], "", 0

These variables do not form a natural tuple. Use three lines of code. Readability counts. Of course there is nothing wrong with initializing e. g. coordinates in a single line.

Do initialize variables right before you need them

In the line

f_list, h_num, p = [], "", 0

the variable h_num is initialized at the beginning of the function while it is needed just before the second loop. Compare the readability of

def dh_func(d_num):
    f_list, h_num, p = [], "", 0

    while d_num > 0:
        f_list.append(d_num % 16)
        d_num //= 16 

    for f in f_list[::-1]:
        h_num += d_func(f, 2)

    return h_num

to

def dh_func(d_num):

    f_list = []
    while d_num > 0:
        f_list.append(d_num % 16)
        d_num //= 16 

    h_num = ""
    for f in f_list[::-1]:
        h_num += d_func(f, 2)

    return h_num

Avoid adding strings

In the second loop of function dh_func (see above) you use + for appending to a string. This is a inefficient operation in python. There is the string method join() for that task. So we rewrite the function with a better function name to

def to_hex(n):
    l = []
    while n > 0:
        l.append(digit[n%16])
        n //= 16
    return "".join(l[::-1])
\$\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.