# Custom checksum algorithm in Python

How can I make the following checksum algorithm more pythonic and readable?

items = {1:'one', 2:'two', 3:'three'}
text = "foo" # This variable may change on runtime. Added to allow the execution
number = 1 # This variable may change on runtime. Added to allow the execution

string = text + items[number] + " " + "trailing"
if items[number] == "two": string=text
string = [ord(x) for x in string]
iterator = zip(string[1:], string)
checksum = string[0]
for counter, (x, y) in enumerate(iterator):
if counter % 2:
checksum += x
checksum += 2 * y
else:
checksum += x * y
checksum %= 0x10000

print("Checksum:", checksum)

The thing called salt by everyone here is fixed and won't change ever. Also, the code is into a bigger function.

This code as-is must return this:

Checksum: 9167
• I don't know how this got 3 up votes. The code does not run as it is. More, we don't have any context about what this code is supposed to do. Could you please edit your question and add more context / the extra piece of code you omitted ? Feb 7, 2017 at 16:15
• @Dex'ter: Sorry. This happens when you extract a code snippet and then generalize it with laziness. I neither know how does this have 3 up votes. Feb 7, 2017 at 16:29
• @Dex'ter Well, it ran for me when setting str_a and str_b to some strings ... Feb 7, 2017 at 16:35

First of all, I would apply the "Extract Method" refactoring method and put the logic behind generating a checksum into a get_checksum() function.

Also, I would avoid hardcoding the trailing "salt" and the 0x10000 "normalizer" and put them into the default keyword argument value and to a "constant" respectively.

You can use sum() instead of having loop with if and else, which increase the overall nestedness. But, it might become less explicit and potentially a bit more difficult to understand.

Comments and docstrings would definitely be needed to describe the logic behind the checksum calculations.

Variable naming can be improved - str_a, str_b and string are not the best choices (even though the part1 and part2 in the code below are not either - think about what these strings represent - may be you'll come up with better variable names). Also, note that you are reusing the string variable to keep the "ords" of each character, which, strictly speaking, is not good - the variable name does not correspond to what it is used for.

At the end you would have something like:

DEFAULT_SALT = "trailing"
NORMALIZER = 0x10000

def get_checksum(part1, part2, salt=DEFAULT_SALT):
"""Returns a checksum of two strings."""

combined_string = part1 + part2 + " " + salt if part2 != "***" else part1
ords = [ord(x) for x in combined_string]

checksum = ords[0]  # initial value

# TODO: document the logic behind the checksum calculations
iterator = zip(ords[1:], ords)
checksum += sum(x + 2 * y if counter % 2 else x * y
for counter, (x, y) in enumerate(iterator))
checksum %= NORMALIZER

return checksum

We can see that the test you've mentioned in the question passes:

\$ ipython3 -i test.py
In [1]: get_checksum("foo", "one")
Out[1]: 9167
• Can you update the answer with the new code? I've made a edit after the Dex' ter comment. Feb 7, 2017 at 16:38
• @Helio You can use his (and also my) function directly, even with your new code, by just passing it the correct arguments. Feb 7, 2017 at 16:39
• @Graipher: Yes. P. S: You made a typo on my nickname. ;-) Feb 7, 2017 at 16:41
• @Helio Hah, it was not yet too late to fix it :) Feb 7, 2017 at 16:41
• part1becomes name, part2 will take one of these values {1:'ver_one', 2:'', 3:'ver_three'} and salt is  version. The "salt" can be constant. ;-) Feb 7, 2017 at 16:46

Similarly to alecxe's answer, i would wrap the code in a function and make the salt a parameter.

I would use str.format for building the string (and still use a ternary).

I see no need for the explicit iterator variable, just inline it.

def checksum(str_a, str_b, salt):
string = str_a if str_b == "***" else "{}{} {}".format(str_a, str_b, salt)
string = [ord(x) for x in string]
checksum = string[0]
for counter, (x, y) in enumerate(zip(string[1:], string)):
if counter % 2:
checksum += x + 2 * y
else:
checksum += x * y
return checksum % 0x10000

Okay so i did read this question few hours ago on phone and started working out an answer on paper, having the performance tag in mind. So here it is:

untouched lines:

items = {1:'one', 2:'two', 3:'three'}
text = "foo" # This variable may change on runtime. Added to allow the execution
number = 1 # This variable may change on runtime. Added to allow the execution

first changes made here. Your Code does set sring and overwrtes it if items[numer] equals string "two". I dont like those inline if elses in long lines so i will take more space then other answers but think this is more readable.

if items[number] == "two":
string = text
else:
string = "{}{} trailing".format(text, items[number])

next line is untouched again with:

string = [ord(x) for x in string]

and now my performance-part comes into play. To understand i will explain your algorith and how i rearanged it. You zip the String starting with the second element and itself. This resolves to [[string[1], string[0]], [string[2], string[1]], ... , [string[last element], string[last element -1]]]. This is a list with len(string) -1 elements.

So we will memorize strings length

string_len = len(string)

We will also memorize if the number of zip-lists elements would be even or odd. In case string has an odd number of elements zip-list would have an even number and if strings number of elements would be even zip-lists had been odd.

odd = (string_len -1) % 2

We will also do the initial setup for checksum as is:

checksum = string[0]

Iterating over iterator there are 2 cases: the iteration is odd

if counter % 2: checksum += x checksum += 2 * y

or its even

else: checksum += x * y

There are two things unnecessary in my opinion. First is creating the zip-list since you can access every element by index. Element x is string[iteration+1], y is string[iteration]. Since counter is successively even and odd we can also say that for two iterations we will once do

checksum += x * y in case our iteration-counter is even and checksum += x checksum += 2 * y in case its not even.

Or we will just allways do 2 iterations in one step and do both operations like:

for i in range(0, string_len -1 -odd, 2):
checksum += string[i] * string[i + 1]  # counter % 2 := false checksum += x * y
checksum += string[i + 2]  # counter % 2 := true checksum += x
checksum += 2 * string[i + 1]  # counter % 2 := true checksum += 2 * y

Now why we did memorize if zip-lists number of elements would had been odd or even: We allways made 2 operations but this wont work for an odd number of operations. So we will check once with an if what to do:

if odd:
checksum += string[i] * string[i + 1]

Anything else as is:

checksum %= 0x10000
print("Checksum:", checksum)

it might be recommendable to save the modulo in another place like other answers mention.

Entire Code looks like this:

items = {1:'one', 2:'two', 3:'three'}
text = "foo" # This variable may change on runtime. Added to allow the execution
number = 1 # This variable may change on runtime. Added to allow the execution

if items[number] == "two":
string = text
else:
string = str.format("{:s}{:s} trailing", text, items[number])

string = [ord(x) for x in string]

string_len = len(string)
odd = (string_len -1) % 2

checksum = string[0]

for i in range(0, string_len -1 -odd, 2):
checksum += string[i] * string[i + 1]  # counter % 2 := false checksum += x * y
checksum += string[i + 2]  # counter % 2 := true checksum += x
checksum += 2 * string[i + 1]  # counter % 2 := true checksum += 2 * y

if odd:
checksum += string[i] * string[i + 1]

checksum %= 0x10000
print("Checksum:", checksum)
• Agree on the ternary being borderline too long to understand. Would write string = "{}{} trailing".format(text, items[number]), though as being shorter and just as easy to read IMO. Also, :s is implied. Feb 7, 2017 at 20:04
• You are absolutely right, somehow i didnt come up with the right method call. Especially this is more OOP which i like, will change it :) Feb 7, 2017 at 21:11

My review (Python 3).

items = {1: 'one', 2: 'two', 3: 'three'}
text = "foo"
number = 1

string_original = "{}{} trailing".format(text, items[number])

if items[number] == "two":
string_original = text

ord_list = [ord(x) for x in string_original]
checksum = ord_list[0]

for i in range(1, len(ord_list)):
x, y = ord_list[i], ord_list[i-1]

# The -1 is necessary since parity is changed by starting the loop on 1
if (i-1) % 2:
checksum = checksum + x + 2*y
else:
checksum = checksum + x*y

checksum %= 0x10000

print("Checksum:", checksum)