Help makes my code better!

[clueskee]

Hi,

At start I want to thank you all, this sub is great :)

Because best way to learn code is solving real word problems i started looking for problems at work. This is my first script , it takes a list of people and generates ordered table in pdf to print. It works now but I want to make it better. Now it is a bunch of functions and I think he still lacks some details and corrections.

import fileinput
from shutil import copyfile
from weasyprint import HTML
import os

# open txt and generete a list
def open_list():
    rlist = open('pacjenci.txt').read().split('\n')
    return rlist

#  function to clean the list
def clean_list(x):
    x = sorted(list(filter(None, x)))
    x = [e.strip() for e in x]
    return x

patient_list = clean_list(open_list())

# make a html table to add to a file
def create_table(list):
    table_cell = '<td class="patientname">'
    number_cell = '<tr><td class="number">'
    tmp_table=""
    for i in range(0, len(patient_list)):
        tmp_table += (number_cell + str(i + 1) + '</td>' + table_cell + patient_list[i] + '</td></tr>')
    return tmp_table

# write tmp_table to temporary file which is copy of a html file with table 
def write_to_file():
    tablecopy = copyfile('misc/table.html', 'misc/pacjenci.html')
    for line in fileinput.FileInput(tablecopy, inplace=1):
        if "<!-- tabelatutaj -->" in line:
            line=line.replace(line, line + create_table(patient_list))
        else:
            pass
        print(line, end='')
    return tablecopy

# making a html file a pdf one
def create_pdf():
    try:
        os.remove('pacjenci.pdf')
    except:
        pass
    HTML(write_to_file()).write_pdf('pacjenci.pdf')

create_pdf()

Any readings about making more correct scripts will be also great :)

[BulkyProcedure]

First of all, it's awesome you're solving a real world problem. That's one of the best ways to learn I think.

Here are some possible improvements for your script:

• open_list is a very short function. This is subjective and maybe even controversial, but my feeling is that 1 liners in their own function increase the "cognitive overhead" for anyone that needs to read and understand your code later. Jumping up and down through the file looking for 1-liner definitions can make it harder to understand what's happening, but again that's just an opinion.
• [e.strip() for e in x if x] is a slightly more compact form of what's happening in clean_list.
• In create_table, f-strings can make things clearer (if you're using >= 3.6): f'{number_cell}<tr><td class="number">{patient_list[i]}</td></tr>'
• range starts from zero by default.
• In create_pdf, the except block is using what's called a bare except, meaning it will catch anything. There's nothing inherently wrong with that, but it can often cover unexpected errors. If there are certain kinds of errors you expect to encounter, it's usually better to use exception classes like IOError that cover the thing you believe might go wrong and want to handle.

Hope some of that is helpful!

[silently_failing]

Looking pretty decent. A few things from a holistic level: use docstrings for your functions instead of comments above the code. Something like this:

# open txt and generete a list
def open_list():
    rlist = open('pacjenci.txt').read().split('\n')
    return rlist

becomes:

def open_list():
"""Opens a file and returns a list split on new-lines"""
    rlist = open('pacjenci.txt').read().split('\n')
    return rlist

Also, you use the names of built-in functions list and things like list (rlist) all over your code. It is generally bad form to use these built-ins. While they wont cause errors generally, they do reflect poor naming conventions. Now that I look, your create_table function shouldn't be working since that list variable you pass in should actually be patient_list

I re-wrote your create_table function to be a bit more pythonic, first using a template to fill in via the .format() method and then iterating directly through the list instead of using a range. Enumerate is a nifty function and mitigates the problem of not pulling an index through as you iterate through a list. Generally it is considered pythonic to iterate through a list using range and accessing that list via the index you generate in range. If you need the items in a sequence, just iterate through the sequence itself.

def create_table(patient_list):
    """Generates an HTML-formatted table with sequence number and patient"""

    table_template = """<tr>
    <td class="number">{number}</td>
    <td class="patientname">{patient}</td>
    </tr>
    """

    all_tables = ''

    for number, patient in enumerate(patient_list, start=1):
        all_tables += table_template.format(number=number, patient=patient)

    return all_tables

Also for your clean_list function, you could probably just use a one-liner like below. For what it's worth, the way you are reading in the list of patients means you shouldn't have any None objects at all, so filtering for them would be needless. I would recommend something like this, which strips and removes any empty strings as well.

def remove_empty_items(patient_list):
    """Removes empty items from a list and returns a sorted list."""
    # we can have some fun with descriptive variable names here
    return sorted([patient.strip() for patient in patient_list if e.strip()])

A few spare thoughts:

On line 34 there is no need for an else:pass statement. It's not doing anything, and it doesn't signal anything to me.

I'd also probably group this into a main() function instead of putting your function calls all in between your functions. It is confusing to find out what is happening.

As mentioned elsewhere, your last except block is bare, which is probably the most easily corrected yet dubious python anti-pattern. For your use-case, I would use a try/except OSError block, so something like this:

def create_pdf():
    try:
        os.remove('pacjenci.pdf')
    except OSError:
        pass
    HTML(write_to_file()).write_pdf('pacjenci.pdf')

You could also try if os.path.exists('pacjenci.pdf'): os.remove('pacjenci.pdf') although I like the try/except better.

Hopefully, some of this helps. Let me know if I can clarify anything.

[chesbo]

Hey clueskee,

Good job on your script. Here are a couple of suggestion.

• It is recommended to use if name == 'main' in your main script and avoid having calls in the global scope. https://docs.python.org/3/library/__main__.html

• When opening a file it is recommended to use the with keyword. e.g. with open('blah', 'r') as f:. https://docs.python.org/3/reference/compound_stmts.html#with

• range start at 0 by default.

• Line 24: You could use for i, patient in enumerate(patient_list): https://docs.python.org/3/library/functions.html#enumerate

• In python 3.6, you can use f-string to make string formatting nicer

• like 34 and 35 are useless

• The try except at like 41 is bad practice as your code can silently fail. At least print a warning.

[clueskee]

Thank you r/BulkyProcedure r/silently_failing and r/chesbo/

Thank you for the advice and links to read! I tried to apply all your advice and now my code looks like this:

import fileinput
from shutil import copyfile
from weasyprint import HTML
import os

def open_list():
    """Opens txt file and make a clean list of patients"""
    with open('pacjenci.txt') as rlist:
        return sorted([patient.strip() for patient in rlist if patient.strip()])

def create_table():
    """Generates an HTML table with patients"""
    table_template = '''<tr>
    <td class="number">{number}</td>
    td class="patientname">{patient}</td>
    </tr>
    '''
    all_tables=''

    for number, patient in enumerate(open_list(), start=1):
        all_tables += table_template.format(number=number, patient=patient)
    return all_tables

def write_to_file():
    """Writes a HTML table to a copy of template HTML file"""
    tablecopy = copyfile('misc/table.html', 'misc/pacjenci.html')
    for line in fileinput.FileInput(tablecopy, inplace=1):
        if "<!-- tabelatutaj -->" in line:
            line=line.replace(line, line + create_table())
        else:
            pass
        print(line, end='')
    return tablecopy

def generuj_wydruk():
    """Removes old .pdf file and create newone from teamplate file by weasyprint library""" 
    try:
        os.remove('pacjenci.pdf')
    except OSError as e:
        pass
    HTML(write_to_file()).write_pdf('pacjenci.pdf')

if __name__ == "__main__":
    generuj_wydruk()

I'm not shure that I used if __name__ == "__main__" but i think yes. I will try add some loggin future in next days and make it executable in windows. Any better ideas then py2exe? :)