4
\$\begingroup\$

I'm new to Python and I'm trying to convert between Microsoft GUIDs and plain hex strings:

def hex_to_guid(hex):
    h = binascii.unhexlify(hex)
    return '-'.join(map(bytes.decode, map(
        binascii.hexlify, (h[0:4][::-1], h[4:6][::-1], h[6:8][::-1], h[8:10], h[10:]))))


def guid_to_hex(guid):
    g = binascii.unhexlify(guid.translate(str.maketrans('', '', '-')))
    return ''.join(map(bytes.decode, map(
        binascii.hexlify, (g[3::-1], g[5:3:-1], g[7:5:-1], g[8:]))))

I suspect this may not be very readable. Am I being too clever?

\$\endgroup\$
1
\$\begingroup\$

This could definitely be easier to follow.

Here are some suggestions:

  • Add some comments and docstrings.

    The single English sentence in your questions is a big help to understanding the purpose of these functions, and where I might find more information, but it’s not attached to the code.

    Something as simple as:

    def hex_to_guid(hex):
        """Convert a hex string to a Microsoft GUID"""
        ...
    

    would be a big improvement. If you ever look back over this code, it’ll be easier if you have this reference alongside the function.

  • Label the parts of the GUID.

    In hex_to_guid, you’re packing too much into that final line, and it’s not obvious what any of the components mean. Working from Microsoft’s specification, I think I was able to work out what all the slices were, and then I put them in named variables:

    # The Microsoft GUID format has four named fields.  Split the
    # bytes into the four fields.  Since the fields are big-endian,
    # we need to reverse the stream returned by unhexlify().
    # https://msdn.microsoft.com/en-us/library/aa373931(VS.85).aspx
    data1 = reversed(h[0:4])
    data2 = reversed(h[4:6])
    data3 = reversed(h[6:8])
    data4a = h[8:10]
    data4b = h[10:]
    
    return '-'.join(map(bytes.decode, map(
        binascii.hexlify, (data1, data2, data3, data4a, data4b))))
    

    Now it’s much easier to check whether the implementation matches the spec.

    (I’m not confident I’ve got this right, but it’s much easier to check when the parts are named and referenced than before.)

  • Prefer list comprehensions to map().

    They’re much more Pythonic, and generally much easier to read. You should also split your code into intermediate steps – it makes it easier to disentangle and debug the individual parts. Something like this seems more natural to me:

    components = [data1, data2, data3, data4a, data4b]
    
    hex_bytes = [binascii.hexlify(d) for d in components]
    decoded = [bytes.decode(b) for b in hex_bytes]
    
    return '-'.join(decoded)
    

    There’s a single logical operation per line of code. You don’t have to unpack complicated map() expressions to read it.

    (My variable names are terrible; you can probably think of something better.)

  • Don’t do too much on a single line.

    You should never do too much on a single line of code. Readability is much improved by splitting operations over multiple lines, and assigning your intermediate results to variables with meaningful names.

    The first line of guid_to_hex could easily be split across two or three lines (possibly with interspersed comments to explain what’s going on). Here’s how I might do it:

    # Remove all dashes from the guid string
    # @@AWLC: I don't know why you went via translate() and maketrans()
    # for something that it seems like replace() would handle.
    guid = guid.replace('-', '')
    
    g = binascii.unhexlify(guid)
    

    You can then name and unpack the components as before.

For what it’s worth, this is what I was left with when I finished writing this answer. It’s substantially longer (although a lot of it is whitespace and prose), but I also claim that a new reader will find my version substantially easier to read, understand and debug:

def hex_to_guid(hex):
    """Convert a hex string to a Microsoft GUID"""
    h = binascii.unhexlify(hex)

    # The Microsoft GUID format has four named fields.  Split the
    # bytes into the four fields.  Since the fields are big-endian,
    # we need to reverse the stream returned by unhexlify().
    # https://msdn.microsoft.com/en-us/library/aa373931(VS.85).aspx
    data1 = reversed(h[0:4])
    data2 = reversed(h[4:6])
    data3 = reversed(h[6:8])
    data4a = h[8:10]
    data4b = h[10:]

    components = [data1, data2, data3, data4a, data4b]

    hex_bytes = [binascii.hexlify(d) for d in components]
    decoded = [bytes.decode(b) for b in hex_bytes]

    return '-'.join(decoded)


def guid_to_hex(guid):
    """Convert a Microsoft GUID to a hex string"""
    guid = guid.replace('-', '')
    g = binascii.unhexlify(guid)

    # As in hex_to_guid, we split the GUID into four named fields.
    data1 = reversed(g[0:3])
    data2 = reversed(g[3:5])
    data3 = reversed(g[5:7])
    data4 = g[8:]

    components = [data1, data2, data3, data4]

    hex_bytes = [binascii.hexlify(d) for d in components]
    decoded = [bytes.decode(b) for b in hex_bytes]

    return '-'.join(decoded)

This is still by no means perfect: there’s still a lot of code repetition in these two functions. Brevity and cleanliness are good, but they’re on a sliding scale with readability: you should find a balance, not an extreme.

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