9
\$\begingroup\$

I wrote the following code to generate a series of barcodes given a CSV with a list of ISBNs in the first column and titles in the second, using the python-barcode library (which itself requires pillow to generate PNGs and JPGs). I am fairly new with Python and have had little formal training, and so would be interested in any and all feedback regarding functionality, formatting, and anything else I might be missing.

import os
import csv
import barcode
from barcode.writer import ImageWriter

def create_code_png(isbn,title):
    """Creates barcode, gives ISBN and title"""
    isbn13 = barcode.get_barcode_class('ean13')
    return isbn13(isbn, writer = ImageWriter())

def remove_forbidden(string_in):
    """Removes characters forbidden from windows file names from a string"""
    forbidden_chars = ":;<>\"\\/?*|."
    return "".join([char for char in string_in if char not in forbidden_chars])

def codes_from_csv(list_location,destination_folder):
    """Creates PNG Barcodes from a csv, placing them in a given folder"""
    with open(list_location, newline='') as csvfile:
        os.chdir(destination_folder)
        for row in csv.reader(csvfile, dialect='excel'):
            code = create_code_png(isbn:=row[0],title:=remove_forbidden(row[1]))
            code.save(isbn + " " + title)

if __name__ == "__main__":
    codes_from_csv("P:\\barcodes.csv","E:\\Barcodes")
\$\endgroup\$
6
\$\begingroup\$

All in all your code looks quite good, it even has documentation! It's interesting to see assignment expressions in actual use, though I have yet to decide if I'm a fan of them. But of course Code Review would not be Code Review if there was nothing to nitpick about ;-)

As per PEP 8,

isbn13(isbn, writer = ImageWriter())

should be

isbn13(isbn, writer=ImageWriter())

whereas

code = create_code_png(isbn:=row[0],title:=remove_forbidden(row[1]))

should be

code = create_code_png(isbn:=row[0], title:=remove_forbidden(row[1]))

We have a little list here on Code Review Meta where you'll find a few tools that can help you to enforce a consistent style, even in larger projects.

Apart from that, you could use str.maketrans/str.translate in remove_forbidden:

# somewhere earlier
FORBIDDEN_SYMBOLS = str.maketrans({letter: "" for letter in ":;<>\"\\/?*|."})

def remove_forbidden(string_in):
     """Removes characters forbidden from windows file names from a string"""
     return string_in.translate(FORBIDDEN_SYMBOLS)
this_is_asuperfancyfilename

By keeping FORBIDDEN_SYMBOLS somewhere outside the function, e.g. as a script level constant, this will be a little bit faster than your original implementation.

If you want to include even more shiny Python features in addition to assignment expressions, maybe also have a look at f-strings (3.6+) and type annotations (3.5+).

I also would not use os.chdir but instead build a full path using either os.path.join (the old way) or the Pathlib module (the new way).

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ I think I'm starting to become less of a fan of assignment expressions, now that I see what kind of (potential) bugs they enable, see my answer :) \$\endgroup\$ – Graipher Feb 21 at 14:21
8
\$\begingroup\$

Since I think this is worth more than just a comment on another answer, in your codes_from_csv you are (ab)using the new walrus operator. Assignment expressions are not the right thing to do here. What they do is assign local variables (outside of the function!), and then the values of those local variables (but not their names) are passed by position to the function.

A small example of how this can lead to unexpected results:

def f(a, b):
    return a // b

# Normal positional arguments, order matters
f(3, 1)
# 3
f(1, 3)
# 0

# Keyword arguments, order does not matter, only name
f(a=3, b=1)
# 3
f(b=1, a=3)
# 3

# Using the walrus operator, variables are assigned,
# but are passed as positional arguments.
# The names of the (external) variables do not matter to the function!
f(a:=3, b:=1)
# 3
f(b:=1, a:=3)
# 0

In other words the last example is equivalent to:

b = 1
a = 3
f(b, a)
# 0

This might be what you want, but since you are using the names of the arguments of the function, it probably isn't. In your code it just so happens to work because you use the same order as in the function definition. This might be a source of bugs at some future point, which could be very hard to find!

|improve this answer|||||
\$\endgroup\$
  • 3
    \$\begingroup\$ But it gets worse: f(b=(a:=1), a=(b:=3)) :-D \$\endgroup\$ – AlexV Feb 21 at 14:28
  • \$\begingroup\$ this doesn't feel particularly more confusing than a = 1 b = 3 f(b=a, a=b) \$\endgroup\$ – TheSaint321 Feb 21 at 20:32

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.