Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite 
def dec_to_bin(ip):

    ip_array = ip.split(".")
    ip_array = filter(None, ip_array)

    if len(ip_array) != 4:
        return "Invalid IP Address format"

    else:
        ip_bin = []

        for x in range(len(ip_array)):

            # Formatting example referenced from: 
            # http://stackoverflow.com/a/10411108/1170681

            ip_bin.append('{0:08b}'.format(int(ip_array[x])))
            ip_bin.append(".")

        ip_bin.pop()
        return ''.join(ip_bin)

This is a simple parser that will take a IP Address in decimal form and convert it into the binary representation.

I'm looking for any tips on coding styles and improving the efficiency of the code.

share|improve this question
Beware: This will not work for IPv6. – luiscubal Dec 8 '12 at 19:16

2 Answers

up vote 5 down vote accepted

Code simplification and related:

Line 4: remove line 3 and replace line 4 with

ip_array = filter(None, ip.split(','))

Line 7: Create an exception, to prevent that a caller gets a string of error instead of the actual result.

Line 12: assuming this is not python3, it's better if you use xrange() instead of range(), because range would create an actual list listing all the items in specified range. xrange(), instead, returns the next item in the sequence without creating the list. This behaviour is the same as the range() function in python3. Anyway, for your purposes changing it with:

for el in ip_array:
    ip_bin.append('{0:08b}'.format(int(el)))

or, even better:

ip_bin = ['{0:08b}'.format(int(el)) for el in ip_array]

Last line: instead of appending the "." after each element, join over the dot:

return '.'.join(ip_bin)

Style: Pursue consistency in all the code: if you use double-quotes for defining strings, use them everywhere, same thing with single-quotes (which i personally prefer for strings, reserving double ones for docstrings).

Reassuming:

def dec_to_bin(ip):

    ip_array = filter(None, ip.split('.'))

    if len(ip_array) != 4:
        raise NotValidIPException('Invalid IP Address format.')
    else:
        ip_bin = ['{0:08b}'.format(int(el)) for el in ip_array]
        return '.'.join(ip_bin)
share|improve this answer
up vote 1 down vote 
def dec_to_bin(ip):

    ip_array = ip.split(".")
    ip_array = filter(None, ip_array)

Why do you need to filter it?

    if len(ip_array) != 4:
        return "Invalid IP Address format"

Never return strings to indicate errors, raise an exception

    else:
        ip_bin = []

        for x in range(len(ip_array)):

Use for ip_element in ip_array:, you should almost never have to iterate over range(len(...)).

            # Formatting example referenced from: 
            # http://stackoverflow.com/a/10411108/1170681

             ip_bin.append('{0:08b}'.format(int(ip_array[x])))

What happens if the user put something else besides numbers in there? I'd suggest matching the ip address against a regular expression to make it more robust.

            ip_bin.append(".")

        ip_bin.pop()
        return ''.join(ip_bin)

Instead use '.'join(ip_bin) and don't try putting the '.' in the list. It'll be simpler.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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