5
\$\begingroup\$

This is some code which prints a hexdump of a list of 16 bit numbers, at 16 bytes distance for each, after the "print data" comment:

#!/usr/bin/python3

# creating test data
data = []
for i in range(500):
    data = data + [ i & 0xff, i >> 8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]

# print data
c = 0
out = ""
for i in range(int(len(data) / 16)):
    if i == 512: break
    low = data[i * 16]
    high = data[i * 16 + 1]
    d = low | (high << 8)
    out = out + ("%04x " % d)
    if (i % 16) == 15: out = out + "\n"
print(out)

It works, but I think I can write it simpler with "join" and maybe list comprehension? But shouldn't be a cryptic one-liner or something, I'm just looking for a more idiomatic Python solution. I want to print the first 512 numbers, but the data array can be shorter or longer.

\$\endgroup\$
8
\$\begingroup\$

Here are some comments on your code:

  • I assume the structure of the input data is to be taken as it is: one long list where only the 1 out of 8 values contribute to the output.
  • c = 0 is defined but never used
  • It is not recommended to use compound statements (see PEP8 - "Other Recommendations"): use a separate line for an if and the statement under condition.
  • / performs a true division (giving a float). To avoid switching between float and int, use integer division: //
  • if i == 512: break could me omitted if you would limit the range of the for loop immediately. Instead of len(data) use min(512*16, len(data))
  • The multiplication i * 16 can be avoided if you use the step argument of range() so that i takes multiples of 16.
  • Instead of "%04x " % d use the newer f-strings
  • Instead of calculating d, you could just pass high and low to the string template and format each independently.
  • Instead of if (i % 16) == 15: you could use a nested loop that deals with one output line
  • Your code produces a blank at the end of each line. That seems unnecessary. With " ".join you would not have this extra blank.
  • out has a terminating \n, but print also prints a newline as terminator (by default). With "\n".joinyou would not have this extra newline

Here is how it could look:

# Set a maximum to the output
length = min(512*16, len(data))

# Format data
lines = []
for line in range(0, length, 256):
    items = []
    for i in range(line, min(line+256, length), 16):
        items.append(f"{data[i+1]:02x}{data[i]:02x}")
    lines.append(" ".join(items))
out = "\n".join(lines)

# Output
print(out)

Here is how the above data formatting translates when using list comprehension. You can split the expression over multiple lines to improve readability:

# Format data
out = "\n".join([
            " ".join([
                f"{data[i+1]:02x}{data[i]:02x}"
                for i in range(line, min(line + 256, length), 16)
            ])
            for line in range(0, length, 256)
        ])
\$\endgroup\$
1
\$\begingroup\$

The review by trinket has covered most, if not all, of the deficits in the code.

The one elephant left in the room is: does out really need to be built up as a single string, just to be printed? Could it not be printed line by line, or even per data value, and not waste time on the string concatenation operations?

Another question is where are these 16-bit values coming from? Are they really entering Python as a list of integers, or perhaps are they coming in as a bytes or bytearray memory buffer type structure? `Cause we can manipulate those to extract the data easier...

data = bytes(data)         # convert from list of ints into a byte array

mv = memoryview(data)      # convert to a memory view...
mv = mv.cast('H')          # treat every 2-bytes as a 16-bit value
mv = mv[:8*512]            # truncate to the first 512 groups of 8 values
mv = mv[::8]               # slice off the first of every 8 values

# Or as one statement...
mv = memoryview(data).cast('H')[:8*512:8]

# Print out each row of the hexdump:
for i in range(0, len(mv), 16):
   print(" ".join(f"{val:04x}" for val in mv[i:i+16]))

# Or construct the big out string, and print as one unit:
out = "\n".join(" ".join(f"{val:04x}" for val in mv[i:i+16])
                for i in range(0, len(mv), 16)
print(out)

Note: The above assumes a little-endian architecture, so that when cast('H') is performed, the correct values are returned. If on a big-endian architecture, the code will need to be modified. See sys.byteorder.

\$\endgroup\$
0
\$\begingroup\$

I like the improvements by @trincot. Another improvement would be to use constants instead of numbers, makes it easier to understand and change instead of using magic numbers scattered in the code. And the f-syntax is nice, but I don't want to install Python 3.6 on my Debian machine, it still runs Python 3.5 and might break things. And I might need the word later for other things as well, and it makes the intention more clear that it is a 16 bit word, so I kept my extra word calculation. And no need to collect all lines in a string, this was just a side product of my solution to avoid multiple lines with resetting the current line and then printing it.

My final code:

max_lines = 32
words_per_line = 16
step = 16
line_length = words_per_line * step
length = min(max_lines * line_length, len(data))
for line in range(0, length, line_length):
    items = []
    for i in range(line, min(line + line_length, length), step):
        d = data[i] + (data[i + 1] << 8)
        items.append("%04x" % d)
    print(" ".join(items))

Will be used for my ADC4 project, which returns 16 bytes per sample, which is the reason for the big step.

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