3
\$\begingroup\$

Please ignore the comments in the code for the purpose of reviewing. I just require tips on using SOLID principles and how classes can be used in this code.Comments in the code are for mapping the image in my brain to what I want to do in words. Sorry for the mistakes in the comments of the code. I wrote them really fast.

This is a follow-up to Use django engine to fill in a .html file on storage (no template) and use weasyPrint to convert it to PDF.

I followed the instructions and refactored my code as per the suggestions by @Reinderien but recently the client said to me that on the webpage there are n ID's and n different PDF's are to be returned based on which ID the user selects. Currently n=2. I have a radio select of which the visitor can select any one of the ID's and enter the correct value of input. So basically I get the value of the radio button as opt and value as the actual ID value. Now I have to check the database to find the column which is named as opt and the row where value in that column is value. Initially there was just one form/word_document/.html_file that was being converted to PDF but now there are three forms. I know it's easy at the moment but I think I need some ideas on whether use of class should be done and if so then how? I 'm really interested in refactoring this code and use SOLID principles. Surely, I 'm beginner level with design patterns and SOLID principles but it's just more and more code reviews that will make my eye catch 'how much' 'single responsibility' a function should be and if really if my function is extensible w/o modifying it. Please assume I 'm good enough at decorator,inheritance and class composition. Also I couldn't get a way to remove the making of temporary files. There is temporary saving of the PDF made by weasyPrint. I don't want that you code it up for me. Just few lines of explanation or a small UML diagram would be more than sufficient.

vars.cfg

[FILES]
HTML_FILE_NAME_FOL=C:\Users\Dell\Desktop\gjh\formfill\media\mydoc-utf8.htm
HTML_FILE_NAME_DPID=C:\Users\Dell\Desktop\gjh\formfill\media\mydoc-utf8.htm
EXCEL_FILE_NAME=C:\Users\Dell\Desktop\gjh\formfill\media\AM2.CSV

forms.py

from django import forms


class InputData(forms.Form):
    opt = forms.ChoiceField(label="Enter Data:", choices=[('FOL', "Folio Number"),
                                                          ('DPID', 'Dpid')
                                                          ],
                            widget=forms.RadioSelect)

    value = forms.CharField(label="Value ", widget=forms.TextInput(
        attrs={'pattern': '[A-Z0-9]{0,16}'}))

backend.py(improved)

import os

import pandas as pd
import codecs
from weasyprint import HTML

import configparser
import tempfile

from django import template
from django.template.loader import render_to_string
from pathlib import Path


def remove_temp_file():
    if os.path.exists('temp.pdf'):
        os.remove('temp.pdf')


def get_config_object():
    config = configparser.RawConfigParser()
    config.optionxform = str
    config.read('vars.cfg')
    return config


config = get_config_object()

def load_custom_tags(opt):

    """ TODO : Django by its nature will only convert a 'template' to HTML.
    For that the file is being saved to the default location that is the template folder.
    I just observed that I do now need to run this function again and again but once once similar 
    to like what one would do if one wants to fill the database the first time.
    Is this possible anyway? I have to give the project folder to the client. He will place all the forms
    paths in the cfg file.

    vars.cfg
    [FILES]
    HTML_FILE_NAME_FOL=C:\Users\Dell\Desktop\gjh\formfill\media\mydoc-utf8.htm
    HTML_FILE_NAME_DPID=C:\Users\Dell\Desktop\gjh\formfill\media\mydoc-utf8.htm
    EXCEL_FILE_NAME=C:\Users\Dell\Desktop\gjh\formfill\media\AM2.CSV
    
    """

    html = codecs.open(
        config["FILES"][f"HTML_FILE_NAME_{opt.upper()}"],
        encoding='utf-8').read()
    if not html.startswith(r"{% load"):
        html += "{% load numbersinwords %}"

    with open(config["FILES"][f"HTML_FILE_NAME_{opt.upper()}"], "w", encoding="utf-8") as html_file:
        html_file.write(html)
    

def html2pdf(row, path_form):
    row = row.to_dict()
    load_custom_tags()
    html = render_to_string(Path(path_form).name,
                            {key: row[value]
                             for key, value in config._sections["TAGS"].items()})
    return html


def get_data():
    return pd.read_csv(config["FILES"]["EXCEL_FILE_NAME"],
                       dtype=str, keep_default_na=False)


def search_row(opt, value):
    user_data = get_data()
    return user_data[user_data[opt] == value]


def main(opt, value):

    remove_temp_file()

    row = search_row(opt, value)

    if len(row) == 1:
        row = row.squeeze()
    else:
        return (False, f"<h1>Invalid credential :"
                " Multiple candidates exists"
                "with given credential</h1>")

    if not(row.empty):
        html = html2pdf(row, Path(config["FILES"][f"HTML_FILE_NAME_{opt.upper()}"]))
        HTML(string=html).write_pdf("temp.pdf")

        f = open("temp.pdf", "rb")

        return (True, f)

    return (False, f"<h1>Invalid credential {opt}: {value}</h1>")
\$\endgroup\$
3
\$\begingroup\$

Configuration file paths

If possible, factor out a common path:

[FILES]
MEDIA_PATH=C:\Users\Dell\Desktop\gjh\formfill\media
HTML_FILE_NAME_FOL=mydoc-utf8.htm
HTML_FILE_NAME_DPID=mydoc-utf8.htm
EXCEL_FILE_NAME=AM2.CSV

Typo?

I do now need -> I do not need

Caching

I do [not] need to run this function again and again but once once similar to like what one would do if one wants to fill the database the first time. Is this possible anyway?

Almost certainly. The easiest way to do this is check if the file exists beforehand. One risk of this approach is that it may not be thread-safe, so you may need to surround the file-check-file-write in a lock if access to that code path is multi-threaded.

Enums

Consider making an Enum to represent the two (?) choices for opt values:

class TagOpt(Enum):
    FOL = 'FOL'
    DPID = 'DPID'

rather than accepting it as a string.

Temporary variables

config["FILES"][f"HTML_FILE_NAME_{opt.upper()}"]

should be put into a temporary variable since you write it twice; perhaps:

html_path = config["FILES"][f"HTML_FILE_NAME_{opt.upper()}"]
html = codecs.open(html_path, encoding='utf-8').read()
with open(html_path, "w", encoding="utf-8") ...

I/O costs

Depending on the load characteristics of your application, you might want to modify load_custom_tags to use something like an LRU cache so that a certain number of most-recently-used HTML files are kept in memory. The cost of a round trip to the hard disc might end up being inconvenient to pay.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Sorry I should have explained it better but soon will with experience on how to present question better. I added those comments so that the reviewer can understand my code better. I ll just add clarification to ignore the comments. Sorry for the trouble @Reinderien \$\endgroup\$ – Vishesh Mangla Jun 29 at 19:21
  • \$\begingroup\$ 1) ENUM :where @Reinderien. In the forms.py or the backend.py? 2)Temporary variables: like temp =config["FILES]; temp[".."]? \$\endgroup\$ – Vishesh Mangla Jun 29 at 19:31
  • \$\begingroup\$ Wow, that was some amazing info there, I didn't know that LRU cache works for file objects too. \$\endgroup\$ – Vishesh Mangla Jun 29 at 19:32
  • \$\begingroup\$ LRU cache works for file objects too - careful. I would not apply an LRU cache to a file handle, but that isn't what's going on here. Instead, you would apply an LRU cache to a string or enum that uniquely identifies the file. \$\endgroup\$ – Reinderien Jun 29 at 19:34
  • \$\begingroup\$ In the forms.py or the backend.py? - backend.py \$\endgroup\$ – Reinderien Jun 29 at 19:35
0
\$\begingroup\$

No need of f strings at places required where no placeholders are there

return (False, f"<h1>Invalid credential :"
        " Multiple candidates exists"
        "with given credential</h1>")

should be simply

return (False, "<h1>Invalid credential :"
            " Multiple candidates exists"
            "with given credential</h1>")

Open close Principle violated:

view.py is dependent on what type of response is being sent back from backend.py. This is bad since if more type of responses are to be returned like HttpResponse, FileResponse, JsonResponse the technique of True False can't be used. You need more than 0/1 in this case. This is not extensible code. The view cares about the response being sent.

This should be better:

view.py

def index(request):
    if request.method == "POST":
        form = InputData(request.POST)
        if form.is_valid():
            return backend.main(**form.cleaned_data)

    form = InputData()

    return render(request, "base/index.html", {
        'forms': form
    })
 

backend.py

import os

import pandas as pd
from weasyprint import HTML

import configparser

from django.http import HttpResponse, FileResponse
from django.template.loader import render_to_string
from django.conf import settings

# --------------load configurations----------------


def get_configurations():
    config = configparser.RawConfigParser()
    config.optionxform = str
    config.read('vars.cfg')
    return config


config = get_configurations()
# --------------load configurations----------------


def remove_temp_file():
    if os.path.exists('temp.pdf'):
        os.remove('temp.pdf')


def get_html(opt, row):

    row = row.to_dict()
    file_path = os.path.join(settings.MEDIA_ROOT,
                             config["FILES"][f"HTML_FILE_NAME_{opt.upper()}"])
    return render_to_string(file_path, context=row)


def search_row_in_database(opt, value):
    df = pd.read_csv(os.path.join(settings.MEDIA_ROOT,
                                  config["FILES"]["EXCEL_FILE_NAME"]),
                     dtype=str, keep_default_na=False)
    return df[df[opt] == value]


def get_pdf(opt, row):
    html = get_html(opt, row)
    HTML(string=html).write_pdf("temp.pdf")
    f = open("temp.pdf", "rb")
    return f


def main(opt, value):

    remove_temp_file()

    row = search_row_in_database(opt, value)

    # check if a single row with that ID exists
    if len(row) == 1:
        row = row.squeeze()
        return FileResponse(
            # opt is required to choose which html pdf is to be picked up
            get_pdf(opt, row),
            as_attachment=True,
            filename=config['DOWNLOAD']['DOWNLOAD_FILE_AS'])
    # no rows with that ID found
    elif len(row) == 0:
        return HttpResponse("<h1>Invalid credential {opt}: {value}. "
                            "No user with that ID found</h1>")
    # in case of not multiple rows with that ID
    else:
        return HttpResponse("<h1>Invalid credential :"
                            " Multiple candidates exists"
                            "with given credential</h1>")

Since the html file is only created once the tag {% load numbersinwords %} should not be put like this.It should be put on creation of the html file manually. These tags can be stored in a readme.md often used with git. The user of the code should then read the readme and put the tag at the top of his html form rather than you doing it programmatically.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I expect such type of answers to this question. Obviously I' m not referring to the f strings . \$\endgroup\$ – Vishesh Mangla Jul 2 at 21:53

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.