10
\$\begingroup\$

I made a small script to read text file that contain hexadecimal and convert to another file in decimal by using command-line as a filename input.

#this is content from hex.txt
0x5B03FA01
0x42018360
0x9FF943B3

and this is what I expected my python to deliver output

#this is content from hex_1.txt
1526987265
1107395424
2683913139

After 3 hour of trial and error. I can see a result in the way I want. The code is follow;

#this was copied from Learn Python the Hard way exercise.
from sys import argv
from os.path import exists

#this is the main function to convert hexadecimal to decimal
def hex2dec (hex):
    result_dec = int(hex, 0)
    return result_dec

#this to accept parameter from command-line
script, from_file = argv

#this to make sure the output generate as a new file with _1
to_file = from_file[:-4] + "_1.txt"
out_file = open(to_file, 'w').close()

#this took me a few hour to figure out a bug. Thanks to stackoverflow
with open(from_file) as in_file:
    lines = in_file.read().splitlines()

for i in lines:
    converted = str(hex2dec (i))
    out_file = open(to_file, 'a+')
    out_file.write(converted + "\n")
    out_file.close()

    #this to print the result for reference on-screen.
    print "converted =" + str(converted)

print "done."

Even though this is done but I feel there are plenty area I could improve to this script e.g. bugs handling in hex2dec function.

However, may I ask you what would you do to enhance this further? and if you could suggest, could anyone suggest me which topic or area I can study further to improve this code or any other about Python?

\$\endgroup\$
2
  • \$\begingroup\$ sh -c 'while read x; do printf "%d\n" $x; done' < hex.txt \$\endgroup\$
    – Alnitak
    Commented Jan 16, 2017 at 14:01
  • \$\begingroup\$ or (more efficiently, since it doesn't invoke the printf command once per line): awk '{ printf "%d\n", $1 }' < hex.txt \$\endgroup\$
    – Alnitak
    Commented Jan 16, 2017 at 14:03

3 Answers 3

8
\$\begingroup\$

Python 3

The script currently does not work in Python 3. For example, print "converted =" + str(converted) is invalid syntax in Python 3, use print() instead (as a function).

Comments

It's great that you document the code. Some comments however don't add any value for future reading: #this took me a few hour to figure out a bug. Thanks to stackoverflow. I have no idea what the bug is, and what the answer from StackOverflow was.

Try to use docstrings as well. For instance in the hex2dec function:

def hex2dec(hex):
    """Convert a hexadecimal string to a decimal number"""
    result_dec = int(hex, 0)
    return result_dec

Some other things about the above function:

  • Rather than hex2dec, perhaps use hex_to_decimal. In this case it's not too big of a deal though.
  • You're storing the result in a variable and then returning the variable without doing anything else to it. This means you could also return result immediately: return int(hex, 0)
  • Since it's using a built in function without any other operation, is the function necessary?

Running it!

As said above, using Python 3 failed to run it. Changing the print statements to functions fixed this issue. Python 2 also supports the print() function, so it should have no issue running on Python 2 as well.

The program ran as expected with a correct file, and produced an output file with the converted values.

Running it with an invalid file, or no arguments at all, crashes the program with either a FileNotFoundError or ValueError respectively. If a file was given which exists but has an invalid hexadecimal value in it (for instance a random string), the program also crashes:

Traceback (most recent call last):
  File "hex2dec.py", line 22, in <module>
    converted = str(hex2dec (i))
  File "hex2dec.py", line 7, in hex2dec
    result_dec = int(hex, 0)
ValueError: invalid literal for int() with base 0: 'random word'

The above errors tells us we need some error checking.

The first error to solve would be the ValueError of not giving a filename as argument when running the program. This can be solved by using a CLI module such as argparse, click, or perhaps another one you like. I personally use argparse, since it's pretty simple and it's part of the standard library.

The arguments:

import argparse


def get_cli_parser():
    parser = argparse.ArgumentParser(description='Describe what your program does')
    parser.add_argument('input_file', type=str,
                        help='The file to read the hexadecimal values from'
    return parser


if __name__ == '__main__':
    parser = get_cli_parser()
    args = vars(parser.parse_args())

At this point you can access the command line arguments args as a dictionary.

The if __name__ == '__main__': part of the code is only applicable if it's run as a script. This means that if the file is imported as a module, this will not run. See also this answer for more info.

When we run the above code without any arguments, we get the following:

usage: hex2dec2.py [-h] input_file
hex2dec2.py: error: the following arguments are required: input_file

Next up is the FileNotFoundError. This one is quite easy to solve: before opening the file, check if it exists using the already imported os module:

if not os.path.exists(args['input_file']):
    print('Input file does not exist, please try again')
    return

The last ValueError can be solved in a number of ways. One would be to use regex on the current line of the file to find any hexadecimal values. Assuming only the hexadecimal values should be on a line, it becomes a lot simpler though. We could just check if the line starts with 0x (e.g. a hexadecimal value).

Reading the file should also be done line by line as an iterator, to prevent huge files from crashing your program by too high memory consumption. Right now the program reads all the text in the file into memory. This can be done as described in the following question:

with open(input_file) as input_file:
    for line in input_file:
        # rest of code here.

Placing this in a function of its own also prevents you from opening the file continuously.

The final product

import argparse
import os


def write_and_print_result(args, result):
    """Write every result to the output file 
    and print each converted value
    """
    out_file = args['input_file'][:-4] + "_1.txt"
    with open(out_file, 'a+') as output:
        for converted in result:
            output.write(converted + "\n")
            print('Converted: {}'.format(converted))


def read_and_convert_file(args):
    """Read the given input file and convert each hexadecimal value
    to decimal value
    """
    result = []
    with open(args['input_file']) as input_file:
        for line in input_file:
            if (line.startswith('0x')):
                result.append(str(int(line, 0)))
    write_and_print_result(args, result)


def get_cli_parser():
    parser = argparse.ArgumentParser(description="Describe program here")
    parser.add_argument('input_file', type=str,
                        help='The file to read the hexadecimal values from')
    return parser


def cli_runner():
    parser = get_cli_parser()
    args = vars(parser.parse_args())

    if not os.path.exists(args['input_file']):
        print('Input file does not exist, please try again.')
        return

    read_and_convert_file(args)


if __name__ == '__main__':
    cli_runner()

As you can see, most of it is either error checking or command line arguments. The actual conversion is only one line of code.

What's next

One issue I still found:

  • If the input file is anything but *.txt, the output file might have a weird name. For example, a file called hex would simply become _1.txt, which isn't very descriptive. To fix this, as pointed out by @Mathias Ettinger, you could use os.path.splitext. It is used as such:
filename, extension = os.path.splitext(args['input_file'])
outfile = filename + "_1.txt"
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your replied, Randyr Note taken: 1. Comment to add value 2. study docstrings 3. use underscroll and word to name function 4. study Python 3 (soon) 5. error handling with argparse, check path/input. \$\endgroup\$
    – Smith Lo
    Commented Jan 16, 2017 at 13:29
5
\$\begingroup\$

You never use os.path.exists, so I removed it. If you want to add some error-handling, you might want to re-add it, though.

I renamed your converter function to adhere to Python's official style-guide, PEP8. It is now called hext_to_dec.

Even though passing 0 to int to make it guess the base is a nice feature of int, here you explicitly know the base it is supposed to be in. You want it to raise an exception if the string being passed to the converter is not a legal base-16 number. Also, explicit is better than implicit.

You should use with for both files, input and output. This way you also avoid re-opening the file every loop iteration. You can also just use 'w' mode now and don't need to open and close the file once at the beginning to make it empty. Note that you can put multiple open calls n the same line when using with.

To iterate over the lines of a file, you can just do for line in in_file:.

You should add docstrings to your functions to document what they do.

By adding a if __name__ == "__main__": guard to your file, it becomes easy to import the function in another script.

converted is already a str, no need to cast it twice.

You can pass multiple arguments to print and it will join them with spaces (and in Python 3.x you can override it to use whatever you want).

from sys import argv

def hex_to_dec(hex):
    """Convert hexadecimal to decimal"""
    return int(hex, 16)

if __name__ == "__main__":
    # this to accept parameter from command-line
    from_file = argv[1]
    to_file = from_file[:-4] + "_1.txt"

    with open(from_file) as in_file, open(to_file, 'w') as out_file:
        for i in in_file:
            converted = str(hex_to_dec(i))
            out_file.write(converted + "\n")
            print "converted =", converted

You could think about accumulating converted numbers and write them all out at once using out_file.writelines([...]). This will be faster, but need more memory (because you need to keep all values in memory). writelines can take any iterable, so you can use a generator expression:

with open(from_file) as in_file, open(to_file, 'w') as out_file:
    out_file.writelines(str(hex_to_dec(i)) + '\n' for i in in_file)

This is much more compact and easier to read, but lacks the printing of the numbers.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your replied, Graipher. Note taken: 1. read PEP8 2. Make it explicitly clear for int function by putting base 16 rather than 0 3. Use with when open file to avoid forget to .close() file. 4. I can use for line in in_file: rather than for string in in_file. 5. study docstrings (PEP 257) 6. study .writelines[...] -- whoa this can make the code very compact. \$\endgroup\$
    – Smith Lo
    Commented Jan 16, 2017 at 13:21
-3
\$\begingroup\$

I would go with @Randyr answer but I would change the read and convert method as follows for two reasons; 1) the output method requires storing all your answers when they wont be processed again and 2) good python apologises rather then asks for permission.

def read_and_convert_file(args):
    """Read the given input file and convert each hexadecimal value
    to decimal value and output to file
    """
    line_num = 0
    out_file = args['input_file'] + ".2decimal"
    with open(out_file, 'a+') as output:
        with open(args['input_file']) as input_file:
            for line in input_file:
                line_num += 1
                try: 
                    converted = str(int(line, 0))
                    output.write(converted + "\n")
                    print('Converted: {}'.format(converted))
                except ValueError:
                    print('Invalid text on line',line_num)
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Since you're promoting EAFP, you should at least make your except guard against a specific exception. \$\endgroup\$
    – Peilonrayz
    Commented Jan 16, 2017 at 16:15
  • \$\begingroup\$ @Peilonrayz - Why? The only ambiguous situation is if the write throws an I/O exception, which while possible, hardly warrants a separate Exception block. It is not like we are writing in Java. \$\endgroup\$
    – Paul Smith
    Commented Jan 16, 2017 at 17:52
  • 2
    \$\begingroup\$ Users can cause a KeyboardInterrupt. PEP8 says "When catching exceptions, mention specific exceptions whenever possible instead of using a bare except: clause.", due to situations like this where you're promoting users to mask bugs. Also where in my previous comment did I mention adding another exception block? \$\endgroup\$
    – Peilonrayz
    Commented Jan 16, 2017 at 18:15
  • 1
    \$\begingroup\$ Although this avoids having to store the converted values, it: 1. makes it harder to test. If you return the list of converted results, it'd be pretty easy to test. 2. It becomes unclear what all the responsibilities of the function are. Now it's both reading, and writing and returns nothing at all. If you want to avoid storing all the results, perhaps it'd be better to turn the read_and_convert_file into a generator which returns only converted values. You could then loop through the converted values and send them to the writing function in real-time. \$\endgroup\$
    – randyr
    Commented Jan 18, 2017 at 12:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.