5
\$\begingroup\$

I am new to python so tips to any part of the code are welcome.

My main problem is how to "overload" the function properly. Is this how supposed to be done with parameters that gets passed exculsivly? It is intended that only Bytes, Words or a DWord will get passed in the corresponding parameter. (If multiple are passed the longest one is prefered. Sidenote: The naming is based on an old 16bit processor thats why word is 16 bit and Dword is 32)

I also wanted to make the range check of the optional arguments in the beginning but as I default them to None, I had to check for None in the first place before I can check the range and this would make the None check redundant.

I had something like max(Word1,Word2) >= 2**16 in mind, because I hoped in python the max would yield None and the larger would give false, but > is not implemented for None. So to get it to work I used the check for None first. But this seems not elegant.

Afterwards I assign the bigger datachunks to Bytes and raise Errors if the data didn't fit in the range of the type. If nothing is passed the Data is set to 0 to fill up the array in the end.

If no length was given the length calculated by the checks is used.

In the end all the data gets packed into a bytearray (the Message to be sent later). Because bytearray() checks if the elements are in range, the Bytes are not checked seperatly.

Is there a more pythonic way to do it?

def packMessage(DeviceID, Offset, dataLength=None,
                Byte0=None, Byte1=None, Byte2=None, Byte3=None,
                Word0=None, Word1=None, DWord0=None):

    Bytes = [Byte0, Byte1, Byte2, Byte3]
    length = 0
    # check if the values to be used are in range
    if Offset >= 2**16:
        raise ValueError
    else:
        Offset_low = Offset & 0x00FF
        Offset_high = (Offset & 0xFF00) >> 8

    if dataLength is not None and not 1 <= dataLength <= 4:
        raise ValueError

    # split bigger data to bytes and check the data length
    if DWord0 is not None:
        if 0 <= DWord0 <= 2**32:
            Bytes[0] = DWord0 & 0xFF
            Bytes[1] = (DWord0 & 0xFF00) >> 8
            Bytes[2] = (DWord0 & 0xFF0000) >> 16
            Bytes[3] = (DWord0 & 0xFF000000) >> 24
            length = 4
        else:
            raise ValueError
    elif Word0 is not None:
        if 0 <= Word0 <= 2**16:
            Bytes[0] = Word0 & 0xFF
            Bytes[1] = (Word0 & 0xFF00) >> 8
        else:
            raise ValueError
        if Word1 is not None:
            if 0 <= Word1 <= 2**16:
                Bytes[2] = Word1 & 0xFF
                Bytes[3] = (Word1 & 0xFF00) >> 8
                length = 4
            else:
                raise ValueError
        else:
            Word1 = 0
            Bytes[2] = Bytes[3] = 0
            length = 2
    else:
        for i in range(len(Bytes)):
            if Bytes[i] is not None:
                length = i
            else:
                Bytes[i] = 0

    # if no length given take calculated length
    if dataLength is None:
        dataLength = length

    # return the Message
    return bytearray([DeviceID, dataLength, Offset_low,
                      Offset_high, Bytes[0], Bytes[1], Bytes[2], Bytes[3]])

Side note: I decided that the compare order of the Bytes doesn't matter that much so I ditched it for now and replaced it with a loop for now.

It looked like this before:

if Byte0 is not None:
    if Byte1 is not None:
        if Byte2 is not None:
            if Byte3 is not None:
                length = 4
            else:
                Byte3 = 0
        else:
            length = 3
            Byte2 = 0
            Byte3 = 0
    else:
        length = 2
        Byte1 = 0
        Byte2 = 0
        Byte3 = 0
else:
    length = 1
    Byte0 = 0
    Byte1 = 0
    Byte2 = 0
    Byte3 = 0

I don't see a nice way to wrap this up. So If there is a beautiful solution for this let me know, otherwise feel free to ignore it.

\$\endgroup\$
  • \$\begingroup\$ @MathiasEttinger I did some rework of the code and also expanded the description. I would appreciate a quick feedback if my post now meets the requirements. \$\endgroup\$ – Kami Kaze Mar 20 '18 at 8:10
  • 1
    \$\begingroup\$ Sounds about right, I hope you get some good answers. \$\endgroup\$ – Mathias Ettinger Mar 20 '18 at 8:56
1
\$\begingroup\$

Simplest would be to separate parsing of the bytes, words and dwords to their own respective method, this would simplify the main logic a lot

def pack_message2(DeviceID, Offset, dataLength=None,
                Byte0=None, Byte1=None, Byte2=None, Byte3=None,
                Word0=None, Word1=None, DWord0=None):

    Offset_low, Offset_high = parse_offset(Offset)

    if dataLength is not None and not 1 <= dataLength <= 4:
        raise ValueError('dataLength should be between 1 and 4')

    result = parse_dword(DWord0)
    if result == ():
#         print('no dwords')
        result = tuple(parse_words(Word0, Word1))  
        if result == ():
#             print('no words')
            result = parse_bytes(Byte0, Byte1, Byte2, Byte3)

    if dataLength is None:
        dataLength = len(result)

    result = pad_tuple_to(result, 4, 0) # if result should be padded with 0's'
#     print(result, dataLength)
    return bytearray([DeviceID, dataLength, Offset_low, Offset_high, *result])

Even if you wouldn't know what the implementation of each of these methods were, the code would be clear in what happens

offset

parsing the offset is pretty straightforward

def parse_offset(Offset):
    if Offset >= 2**16:
        raise ValueError('Offset should be less than 2**16')
    return (Offset & 0x00FF), ((Offset & 0xFF00) >> 8)

Data

Instead of manually chaining the splitting the words in bytes, you can use that generator expression. This will also allow you to easily parse triple words or even quads without letting the code explode

def parse_dword(dword):
#     print('dword:', dword)
    if dword is None:
        return ()
    if not 0 <= dword <= 2**32:
        raise ValueError('dword should be between 0 and 2**32')
    return tuple((dword & 0xFF * (2**(8*i))) >> (8 * i) for i in range(4))

def parse_words(*words):
#     print('words: ', words)
    for word in words:
        if word is None:
#             yield from (0, 0)
            break
        if not 0 <= word  <= 2**16:
            raise ValueError('word should be between 0 and 2**32')
        yield from ((word & 0xFF * 2 ** (8*i)) >> (8 * i) for i in range(2))

from itertools import takewhile
def parse_bytes(*bytes_):
#     print('bytes: ', bytes_)
    return tuple(takewhile(lambda x: x is not None, bytes_))

These methods can be further generalized, but I'll leave that open as an exercise

padding the result

the simplest (but not purest) way to pad a tuple

def pad_tuple_to(tup, to, default=None):
    return tup + tuple(default for _ in range(to - len(tup)))

generalization

I was intrigued by the generalization, so I tried it myself

def generalized_parse_word(size, words):
#     print('words: ', words)
    if words is None:
        return 'empty'
    for word in words:
#         print('word: ', word)
        if word is None:
#             yield from (0, 0)
            break
        if not 0 <= word  <= 2 ** (8 * size):
            raise ValueError('word with %i should be between 0 and %i' % (size, 2 **(8*size)))
        yield from ((word & 0xFF << (8*i)) >> (8 * i) for i in range(size))

def pack_message3(DeviceID, Offset, dataLength=None,
                Byte0=None, Byte1=None, Byte2=None, Byte3=None,
                Word0=None, Word1=None, DWord0=None):

    Offset_low, Offset_high = parse_offset(Offset)

    if dataLength is not None and not 1 <= dataLength <= 4:
        raise ValueError('dataLength should be between 1 and 4')

    data = (
        (4, (DWord0,)),
        (2, (Word0, Word1)),
        (1, (Byte0, Byte1, Byte2, Byte3)),
    )

    for size, words in data:
        result = tuple(generalized_parse_word(size, words))
        if result != ():
            break

    if dataLength is None:
        dataLength = len(result)

    result = pad_tuple_to(result, 4, 0) # if result should be padded with 0's'
#     print(result, dataLength)
    return bytearray([DeviceID, dataLength, Offset_low, Offset_high, *result])

arguments

Instead of manually naming the different bytes or words, I would accept a collection as argument, something like this:

def pack_message4(DeviceID, Offset, dataLength=None,
                dwords=None, words=None, bytes_=None):
    ...
    data = ((4, dwords), (2, words), (1, bytes)) 
    ...

testing

inputs = [
    {'Word0': 0x1122, 'Word1': 0x3344},
    {'DWord0': 0x11223344},
    {'Byte0':34, 'Byte1':17, 'Byte2':68, 'Byte3': 51},
]
for i in inputs:
    ans0 = packMessage(DeviceID=3, Offset=220, **i)
    ans1 = pack_message3(DeviceID=3, Offset=220, **i)
    print(ans0 == ans1, ans0, ans1)
True bytearray(b'\x03\x04\xdc\x00"\x11D3') bytearray(b'\x03\x04\xdc\x00"\x11D3')
True bytearray(b'\x03\x04\xdc\x00D3"\x11') bytearray(b'\x03\x04\xdc\x00D3"\x11')
False bytearray(b'\x03\x03\xdc\x00"\x11D3') bytearray(b'\x03\x04\xdc\x00"\x11D3')

Could it be your method to calculate the dataLength in the case of Bytes is off by 1

\$\endgroup\$
  • \$\begingroup\$ Thank you for the review. As I am working on another project with a more pressing deadline at the moment, I was not able to read through your suggestions, yet. I will do so as soon as I am returning to the project and will then give my vote and if applicable accept the answer. \$\endgroup\$ – Kami Kaze Mar 27 '18 at 14:16
  • \$\begingroup\$ Okay putting bits of code in a function was the more or less next logical step. I have problems with the vast amount of data types and their appropiate uses, so this might help me get a better feeling. The optimizations possible by just using another type kind of overwhelm me. \$\endgroup\$ – Kami Kaze Apr 12 '18 at 14:24
  • \$\begingroup\$ *result is unpacking the tuple to a list? \$\endgroup\$ – Kami Kaze Apr 12 '18 at 14:56
  • \$\begingroup\$ you can try it with something like this ´a = (3, 4); print([1, 2, *a, 5])´ \$\endgroup\$ – Maarten Fabré Apr 12 '18 at 19:43
  • \$\begingroup\$ what is the default parameter in the pad_tuple_to for? \$\endgroup\$ – Kami Kaze Apr 19 '18 at 11:38

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.