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

2 Answers 2

4
\$\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\$
2
\$\begingroup\$

A python3 compatible version, leveraging struct and itertools

note that I've changed the names of the functions, as hex_to_guid is a misnomer if the input is in bytes, as is the case in the question.


import struct
from itertool import chain

def bytes_to_guid(byte_string):
    """Convert a byte string to a Microsoft GUID"""
    
    # The Microsoft GUID format has five fields separated by dashes.  
    # since the endianness is mixed, we need to reverse the first three fields. 
    # https://en.wikipedia.org/wiki/Universally_unique_identifier#Encoding
    
    # big endian components, we swap the endianness 
    A = (bytes(reversed(component))
         for component
         in struct.unpack_from('4s2s2s', byte_string, 0))

    # little endian components, good as is. 
    B = struct.unpack_from('2s6s', byte_string, 8)

    return '-'.join(c.hex() for c in chain(A,B))


def guid_to_bytes(guid):
    """Convert a Microsoft GUID to a byte string"""
    components = [bytes.fromhex(c) for c in guid.split('-')]
    
    # big endian components, we swap the endianness 
    A = (bytes(reversed(component)) 
         for component 
         in components[:3])
    
    # little endian components, good as is. 
    B = components[3:]
    
    return b''.join(c for c in chain(A,B))
```
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ Mar 14 at 7:33
  • \$\begingroup\$ There are enough comments which explain the feedback. Legit answer. \$\endgroup\$ Mar 14 at 8:46
  • 1
    \$\begingroup\$ @BillalBegueradj The comments seem to be referring to another answer and not the original post. \$\endgroup\$
    – pacmaninbw
    Mar 14 at 13:41
  • \$\begingroup\$ I think the previous answer documented enough of the issues with the original code, here I'm mostly concerned with updating the code to modern python. Is that a valid strategy? \$\endgroup\$
    – A-y
    Mar 14 at 17:19

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.