5
\$\begingroup\$

The backstory (you can skip this if you want to)

This code stems from a project that I had to do for a presentation for a university class around November 2023. The task was the following:

Create a booking website for online lectures and events with dates

We didn't even have to present the project itself, but things like the development cycle, a requirements analysis, etc. with up to 6 screenshots of the actual website instead.

This also my very first full-stack web app.

Some drawings

Since I did decide to use some diagrams inside my presentation, it may also help to know what I was going for while writing the code.

This is the type of data that I wanted to store:

Diagram of the Tables

This turned out to be a mix of an entity-relationship-model and a class diagram

And here is a diagram of the entire tech stack, so you can draw a picture (no pun intended) of what the code did:

The tech stack

THE ACTUAL THING

The link to my project on GitHub:

https://github.com/JupperTV/ReferatGrWeb/tree/english-translation

I do want you to use the english-translation branch because on that branch I translated every comment and string that was in German into English. Barely anthing related to the actual logic has been changed.

The code that I want to submit for review in here are the 3 managers. These modules are almost identical, but they still have their small differences that may be worth reviewing. The 3 managers are an abstraction layer for each CSV file where they have multiple functions to retrieve, insert, update, and delete data

I did not implement some things because I thought that it may not be worth it.

accountmanager.py:

#!/usr/bin/python

# ! IMPORTANT NOTE: I obfuscate the data instead of doing any encryption
# ! because this application was only created for demonstration purposes
from base64 import b64encode, b64decode  # Zum Obfuskieren der Daten
import csv  # All of the data is stored in CSV files
import re#gex
from typing import Final, Iterable
import uuid  # To create unique ids

import errors

# Source: https://regexr.com/3e48o
_REGEX_VALID_EMAIL: Final = r"^\S+@\S+\.\S+$"
_UNSUCCESFUL_MATCH = None
_CSV_PATH: Final[str] = "data"
_CSV_ACCOUNT: Final[str] = f"{_CSV_PATH}\\accounts.csv"

# Static variables inside a class for better readabilty
class CSVHeader:
    ACCOUNTID: Final[str] = "id"
    EMAIL: Final[str] = "email"
    BASE64PASSWORD: Final[str] = "password"
    FIRSTNAME: Final[str] = "firstname"
    LASTNAME: Final[str] = "lastname"
    def AsList() -> list[str]:
        return [CSVHeader.ACCOUNTID, CSVHeader.EMAIL, CSVHeader.BASE64PASSWORD,
                     CSVHeader.FIRSTNAME, CSVHeader.LASTNAME]

# A class so that, as long as app.py and eventmanager.py get the
# accountid and email from GetAccountFromEmail() or
# GetAccountFromToken(), they can get whatever account values they need
# themselves
class Account:
    def InitFromDict(dictionary: csv.DictReader | dict[str, str]):
        if len(dictionary) < 5:
            raise errors.NotEnoughElementsInListError()
        d = lambda h: dictionary.get(h)  # Less boilerplate
        return Account(
            accountid=d(CSVHeader.ACCOUNTID), email=d(CSVHeader.EMAIL),
            base64password=d(CSVHeader.BASE64PASSWORD),
            firstname=d(CSVHeader.FIRSTNAME), lastname=d(CSVHeader.LASTNAME))


    def __init__(self, accountid: str, email: str, base64password: str,
                 firstname: str, lastname: str):
        self.accountid = accountid
        self.email = email
        self.base64password = base64password
        self.firstname = firstname
        self.lastname = lastname


# region Private functions
# A csv.DictReader basically works like a list[dict[str, str]]
def _getdictreader_() -> csv.DictReader:
    # * FUTURE ME: I believe I wasted 5 hours on this
    # * Weird Python behaviour:
    # The newline parameter in open() is an empty string because
    # csv.writer and csv.reader have their own ways of controlling line
    # breaks (they just embed a "\r\n" by themselves)
    # If the newline parameter is not empty, Windows will turn the
    # "\r\n" that was written by csv.writer into "\r\r\n", which results
    # in an empty line between every data set.
    # Sources:
    # - https://stackoverflow.com/a/3348664
    # - Footnote in https://docs.python.org/3/library/csv.html?highlight=csv.writer#id3
    accountfile_read = open(_CSV_ACCOUNT, "r", newline="")
    return csv.DictReader(accountfile_read, delimiter=",")

def _obfuscateText_(text: bytes) -> bytes:
    if type(text) is str:
        # Unicode instead of UTF-8 because Python 3 uses unicode for
        # strings
        text = bytes(text, encoding="unicode")
    return b64encode(text)
# endregion

# * Note:
# I return an entire instance of Account so that app.py and
# entrymanager.py can get the values they need themselves without me
# always having to create a new function that just returns the value
# that is currently needed.
def GetAccountFromEmail(email: str) -> Account | None:
    reader: csv.DictReader = list(_getdictreader_())
    account = None
    for row in reader:
        # * Important Detail:
        # row.get(...) is being used instead of row[...] because
        # row.get(...) returns `None` if the key, or column, doesn't
        # exist.
        # This is important because the values of the keys will
        # automatically be None if the key/column or even
        # the entire row empty is.
        # (Python's *BUILT-IN* csv library has some unfortunate
        # differences when dealing with line breaks - see comment
        # "Weird Python behaviour" in accountmanager.AddAccount)
        if row.get(CSVHeader.EMAIL) == email:
            return Account.InitFromDict(row)
    raise errors.AccountDoesNotExistError()

# Note: accountid == token stored in the cookie
def GetAccountFromToken(token: str) -> Account:
    reader: csv.DictReader = _getdictreader_()
    for row in reader:
        if row.get(CSVHeader.ACCOUNTID) == token:
            return Account.InitFromDict(row)

def PasswordsAreEqual(originalpassword: str, obfuscatedpassword: str) -> bool:
    return originalpassword == b64decode(obfuscatedpassword).decode()

def LoginIsValid(email: str, originalpassword: str) -> bool:
    # accountfile_read = open(_CSV_ACCOUNT, "r", newline="")
    # reader: Iterable[dict] = csv.DictReader(accountfile_read, delimiter=",")
    reader: csv.DictReader = _getdictreader_()
    for row in reader:
        # Don't check passwords until emails are the same
        if row.get(CSVHeader.EMAIL) != email:
            continue
        if PasswordsAreEqual(originalpassword=originalpassword,
                             obfuscatedpassword=row.get(CSVHeader.BASE64PASSWORD)):
            return True
    return False

# TODO: Test
def UserExists(email: str) -> bool:
    reader = _getdictreader_()
    for row in reader:
        if row.get(CSVHeader.EMAIL) == email:
            return True
    return False

# TODO: Improve in the future?
def PasswordIsValid(originalpassword: str) -> bool:
    if not originalpassword:
        return False
    return True

# The HTML type "email" does everything for me
def EmailIsValid(email: str) -> bool:
    # return re.fullmatch(_REGEX_VALID_EMAIL, email) != _UNSUCCESFUL_MATCH
    return True

def SaveInCSV(email, originalpassword, firstname, lastname) -> None:
    if not PasswordIsValid(originalpassword):
        raise ValueError("Invalid password ")
    if not EmailIsValid(email):
        raise ValueError("Invalid e-mail")
    reader = _getdictreader_()

    for row in reader:
        if email == row.get(CSVHeader.EMAIL):
            raise errors.AccountAlreadyExistsError()

    accountid = uuid.uuid4()  # Random UUID
    with open(_CSV_ACCOUNT, "a", newline='') as accountfile_write:
        writer = csv.DictWriter(accountfile_write, fieldnames=CSVHeader.AsList(),
                                delimiter=",")
        # .decode() comes from the bytes class and turns the bytes
        # object into a str
        passwordToSave = _obfuscateText_(
            bytes(originalpassword, "unicode_escape")).decode()

        # There could be a better way to save the values from the
        # Account object
        values = [accountid, email, passwordToSave, firstname, lastname]
        writer.writerow(dict(zip(CSVHeader.AsList(), values)))

# TODO
def RemoveAccount():
    pass


entrymanager.py:

#!/usr/bin/python
# * What is an Entry?
# An entry is what connects an account with an event.
# If, for example, a user registers for an event, then this is
# is saved as an entry/registration for this event

import csv
from typing import Final, Iterable
import uuid

import eventmanager
import errors

_CSV_PATH: Final[str] = "data"
_CSV_ENTRY: Final[str] = f"{_CSV_PATH}\\entries.csv"

# * Note:
# * I don't have a class for the entries, like I did for the accounts
# * and the events, because a dataset for an entry only consists of 3
# * ids anyway.

class CSVHeader:
    ENTRYID: Final[str] = "id"
    ACCOUNTID: Final[str] = "accountid"
    EVENTID: Final[str] = "eventid"
    def AsList() -> list[str]:
        return [CSVHeader.ENTRYID, CSVHeader.ACCOUNTID, CSVHeader.EVENTID]

def _getdictreader_() -> csv.DictReader:
    entryfile_read = open(_CSV_ENTRY, "r", newline="")
    return csv.DictReader(entryfile_read, delimiter=",")

def SaveInCSV(accountid: int, eventid: int) -> None:
    entryfile_writer = open(_CSV_ENTRY, "a", newline="")
    # A Dictwriter isn't worth it here because I would have to zip the 3
    # variables with CSVHeader.AsList() and that's too much work for so
    # little
    writer = csv.writer(entryfile_writer, delimiter=",")
    entryid = uuid.uuid4()
    writer.writerow([entryid, accountid, eventid])

def DidAccountAlreadyEnter(accountid, eventid) -> bool:
    reader = _getdictreader_()
    for row in reader:
        if not row.values():  # row is empty
            continue
        if row.get(CSVHeader.ACCOUNTID) == accountid \
            and row.get(CSVHeader.EVENTID) == eventid:
            return True
    return False

def GetAllEntriedEventsOfAccount(accountid) -> list[eventmanager.Event] | None:
    events: list[eventmanager.Event] = eventmanager.GetAllEvents()
    reader = _getdictreader_()
    if not reader:
        return None
    entriedevents: list[eventmanager.Event] = []
    for row in reader:
        if row.values() and row.get(CSVHeader.ACCOUNTID) == accountid:
            entriedevents.append(eventmanager.GetEventFromId(row.get(CSVHeader.EVENTID)))
    return entriedevents

def DeleteAllEntriesWithEvent(eventid) -> None:
    events: list[eventmanager.Event] = eventmanager.GetAllEvents()
    reader: csv.DictReader = _getdictreader_()
    rowsWithoutEvent: list[dict[str, str]] = []
    for row in reader:
        if not row.values():
            continue
        if row.get(CSVHeader.EVENTID) == eventid:
            continue
        rowsWithoutEvent.append(row)

    # * Important Note:
    # The file will be deleted immediately after being opened.
    # writer.writerows() will completely overwrite it
    entryfile_write = open(_CSV_ENTRY, "w", newline="")
    writer = csv.DictWriter(entryfile_write, fieldnames=CSVHeader.AsList(),
                            delimiter=",")
    writer.writerow(dict(zip(CSVHeader.AsList(), CSVHeader.AsList())))
    writer.writerows(rowsWithoutEvent)

def DeleteEntry(accountid: int, eventid: int) -> None:
    reader = _getdictreader_()
    newCSV: list[dict[str, str]] = []
    for row in reader:
        if not row.values():
            continue # raise errors.AccountHasNoEntriesError("Only Header")
        if row.get(CSVHeader.ACCOUNTID) == accountid and row.get(CSVHeader.EVENTID) == eventid:
            continue
        newCSV.append(row)

    entryfile_write = open(_CSV_ENTRY, "w", newline="")
    writer = csv.DictWriter(entryfile_write, fieldnames=CSVHeader.AsList(),
                            delimiter=",")

    writer.writerow(dict(zip(CSVHeader.AsList(), CSVHeader.AsList())))
    writer.writerows(newCSV)


eventmanager.py:

#!/usr/bin/python

# * What is an event?
# An event is an event that users can register to
# Examples: Lectures, Videocalls, Meetings

import csv
from typing import Final, Iterable
import time
from datetime import datetime
import uuid

import babel.dates

import errors
import entrymanager

class EventType:
    ON_SITE: Final[str] = "onsite"
    ONLINE: Final[str] = "online"

class CSVHeader:
    EVENTID: Final[str] = "id"
    NAME: Final[str] = "name"
    EPOCH: Final[str] = "epoch"
    EVENTTYPE: Final[EventType] = "type"
    ORGANIZER_EMAIL: Final[str] = "organizer"
    COUNTRY: Final[str] = "country"
    CITY: Final[str] = "city"
    ZIPCODE: Final[str] = "zipcode"
    STREET: Final[str] = "street"
    HOUSENUMBER: Final[str] = "housenumber"
    DESCRIPTION: Final[str] = "description"

    # csv.DictWriter needs the fieldnames of the CSV file
    def AsList() -> list[str]:
        return [CSVHeader.EVENTID, CSVHeader.NAME, CSVHeader.EPOCH,
                CSVHeader.EVENTTYPE, CSVHeader.ORGANIZER_EMAIL, CSVHeader.COUNTRY,
                CSVHeader.CITY, CSVHeader.ZIPCODE, CSVHeader.STREET,
                CSVHeader.HOUSENUMBER, CSVHeader.DESCRIPTION]

class Event:
    def InitFromDict(dictionary: csv.DictReader | dict[str, str]):
        if len(dictionary) < 11:
            raise errors.NotEnoughElementsInListError()
        d = lambda h: dictionary.get(h)  # Less boilerplate
        return Event(eventid=d(CSVHeader.EVENTID),
                     eventname=d(CSVHeader.NAME),
                     epoch=d(CSVHeader.EPOCH),
                     eventtype=d(CSVHeader.EVENTTYPE),
                     organizeremail=d(CSVHeader.ORGANIZER_EMAIL),
                     country=d(CSVHeader.COUNTRY),
                     zipcode=d(CSVHeader.ZIPCODE),
                     city=d(CSVHeader.CITY),
                     street=d(CSVHeader.STREET),
                     housenumber=d(CSVHeader.HOUSENUMBER),
                     description=d(CSVHeader.DESCRIPTION)
                     )

    def __init__(self, eventid: str, eventname: str, epoch: str, eventtype: str,
                 organizeremail: str, country: str, city: str, zipcode: str,
                 street: str, housenumber: str, description: str):
        self.eventid = eventid
        self.eventname = eventname
        self.epoch = epoch
        self.eventtype=eventtype
        self.organizeremail = organizeremail
        self.country = country
        self.city = city
        self.zipcode = zipcode
        self.street = street
        self.housenumber = housenumber
        self.description = description

    def __iter__(self):
        return iter([
            self.eventid, self.eventname, self.epoch, self.eventtype,
            self.organizeremail, self.country, self.city, self.zipcode,
            self.street, self.housenumber, self.description])

# This only exists for displaying information on the frontend
KEYS_FOR_OUTPUT = ["Event number", "Eventname", "Date", "Event type",
           "Organizer e-mail", "Country", "City", "Zipcode", "Street",
           "House number", "Description"]

def GetReadableEventType(eventtype: str) -> str:
    return "On site" if eventtype == EventType.ON_SITE else "Online"


_CSV_PATH: Final[str] = "data"
_CSV_EVENT: Final[str] = f"{_CSV_PATH}\\events.csv"

def _getdictreader_() -> csv.DictReader:
    eventfile_read = open(_CSV_EVENT, "r", newline="")
    return csv.DictReader(eventfile_read, delimiter=",")

def EpochToNormalTime(epoch: float | str) -> str:
    readabledate = datetime.fromtimestamp(float(epoch))
    return babel.dates.format_datetime(readabledate, locale="de_DE",
                                       format="dd.MM.yyyy 'um' HH:mm")

def EpochToInputTime(epoch: float | str):
    return time.strftime("%Y-%m-%dT%H:%M", time.localtime(float(epoch)))

def InputTimeToEpoch(inputtime: str):
    return float(time.mktime(time.strptime(inputtime, "%Y-%m-%dT%H:%M")))

def GetAllEventsCreatedByOrganizer(organizeremail: str) -> list[Event]:
    reader = _getdictreader_()
    events: list[Event] = []
    for row in reader:
        if not row.values():
            continue
        if row.get(CSVHeader.ORGANIZER_EMAIL) == organizeremail:
            events.append(Event.InitFromDict(row))
    if not events:
        raise errors.AccountHasNoEventsError()
    return events

def GetAllEvents() -> list[Event]:
    return [Event.InitFromDict(row) for row in _getdictreader_()]

def EventExists(event: Event) -> bool:
    reader = _getdictreader_()
    for row in reader:
        if event == Event.InitFromDict(row):
            return True
    return False

def GetEventFromId(eventid) -> Event | None:
    reader = _getdictreader_()
    for row in reader:
        if eventid == row.get(CSVHeader.EVENTID):
            return Event.InitFromDict(row)
    return None

def IsTheSameEvent(*args) -> bool:
    reader = list(_getdictreader_())
    if not reader:
        return False
    for row in reader:
        # If a user enters the exact same data of another event, then
        # the eventids will be the only thing difference.
        # That's why they aren't being compared
        row.pop(CSVHeader.EVENTID)
        for index, header in enumerate(CSVHeader.AsList()):
            if args[index] != row.get(header):
                return False
    return True

def CreateEventFromForm(eventname, epoch: float, eventtype: str, organizeremail,
                        country, city, zipcode: str, street, housenumber: str,
                        description: str) -> None:
    SaveInCSV(eventid=uuid.uuid4(), eventname=eventname, epoch=epoch,
              eventtype=eventtype, organizeremail=organizeremail, country=country,
              city=city, zipcode=zipcode, street=street, housenumber=housenumber,
              description=description)

def SaveInCSV(eventid, eventname, epoch, eventtype, organizeremail, country, city, zipcode,
              street, housenumber, description) -> None:
    if IsTheSameEvent(eventname, epoch, eventtype, organizeremail, country, city, zipcode,
                      street, housenumber, description, eventtype):
        raise errors.EventAlreadyExistsError()
    eventfile_write = open(_CSV_EVENT, "a", newline="")
    # It's not worth it to use a DictWriter just to insert a new dataset
    writer = csv.writer(eventfile_write, delimiter=",")
    writer.writerow([eventid, eventname, epoch,eventtype, organizeremail,
                     country, city, zipcode, street, housenumber, description])

def ModifyEvent(event: Event) -> None:
    reader: list[dict] = list(_getdictreader_())
    for row in reader:
        if row.get(CSVHeader.EVENTID) == event.eventid:
            for index, header in enumerate(CSVHeader.AsList()):
                reader[reader.index(row)][header] = list(event)[index]
            break  # The event has been found

    eventfile_write = open(_CSV_EVENT, "w", newline="")
    writer = csv.DictWriter(eventfile_write, fieldnames=CSVHeader.AsList())
    writer.writerow(dict(zip(CSVHeader.AsList(), CSVHeader.AsList())))
    writer.writerows(reader)

def DeleteEvent(eventid):
    # Every other row including the headers, other than the row of the
    # event that needs to be deleted, are read and then overwriten to
    # the file
    reader = _getdictreader_()
    newCSV: list[dict[str, str]] = []
    for row in reader:
        if not row.values():  # row is empty
            continue
        if row.get(CSVHeader.EVENTID) == eventid:
            continue
        newCSV.append(row)

    eventfile_write = open(_CSV_EVENT, "w", newline="")
    writer = csv.DictWriter(eventfile_write, fieldnames=CSVHeader.AsList(),
                            delimiter=",")

    writer.writerow(dict(zip(CSVHeader.AsList(), CSVHeader.AsList())))
    writer.writerows(newCSV)

    entrymanager.DeleteAllEntriesWithEvent(eventid)


Notes

While I was developing this for my presentation, I did realise why SQL exists how it can save a lot of time. And if you read some of the comments I wrote, you may see that I regretted using the built-in csv library instead of a library dedicated for data like pandas.

So feel free to criticize me for these decisions.

EDIT: I forgot to make the repository public...

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

choice of libraries

In future projects you might consider consider adopting pandas, so you can take advantage of very convenient functions like read_csv() and to_csv(). It also simplifies searching for matching email or password.

choice of language

identifiers

If there's a chance that a diverse set of collaborators will interact with your code, it is worthwhile to write it in English from the outset, to maximize the chance of mutual understanding. This is similar to why technical conferences on the continent typically announce "the conference language will be English." If you're confident that only Deutschsprachigen will interact with it, then sticking with German is fine.

comments

If you want to lean toward German, better to do it in the comments than with variable and function names.

If feasible, better to write German # comments while sticking with English """docstrings""". In order of visibility, we care about broadly communicating the identifiers, docstrings, and comments. I say this after working with a wide variety of code written by people whose Muttersprach was Portuguese, Spanish, French, Italian, Greek, Slavic tongues including Russian, and Germanic tongues including Dutch. I confess that my ability to interact with code goes way down when the docstrings are rendered in any of the Asian scripts.

user output

Please feel free to leave user interactions in the original language if there's no need to support Anglophone end users. Any software engineer will simply take it as a Requirement that the code must output "bonjour tout le monde" instead of "hello world".

That said, I am a tiny bit sad that when translating UX phrases you passed up the opportunity to tackle the bigger goal of supporting multiple languages. It would have introduced you to the various i18n message libraries, and to myriad nightmares like how forming plurals can drastically change when you change languages. Fun times!

I have seen the occasional project which has 50% original German UX phrases and 50% English, which seems rather disconcerting for an end user, so consistency is important. Some errors will trigger seldom, so we might see a 100% English test run on Monday, yet observe some errant German output on Tuesday due to a rare exception.

diagrams

There are many drawing tools, and they can lead to useful ERD and class diagrams that aid initial development. Inevitably they fall out of sync with the evolving codebase and are viewed with increasing suspicion as the months go by.

I recommend starting out by writing stub code, sufficient to get a tool to automatically derive a diagram from your code. Edit those stubs, view the revised diagram, and iterate. The advantage is that, after having produced real working code, you can continue to re-run that workflow to produce up-to-date accurate diagrams which stakeholders will actually trust and believe in.

I did find your diagrams helpful, and I'm glad you included them.

credentials

The password column in the account table troubles me. (BTW, kudos on choosing singular table names rather than plural!)

We should see an argon2id hash in that table instead. End users commonly choose passwords which are low entropy and are re-used across several sites. We wouldn't want breach of your site (or its backup files) to compromise random other web sites, so we store salted hashes instead.

I see that event could be normalized by breaking out the concept of a street address, but that's cool, we can worry about that later if the need arises.

Where you named the modules {account,entry,event}manager.py, more typical practice would be to speak of "models" rather than "managers", no big deal. Also, as they are just simple DAO's with little behavior, you can give each its own module or you may find it more convenient to simply throw them all into models.py, at least when using sqlalchemy.

Oh, dear! As I scan the imports you don't seem to use sqlalchemy. Well, maybe on a subsequent project.

These modules are almost identical ...

Yup. That is exactly the kind of thing that sqlalchemy strives to abstract out of each DB-based project, as similar concerns crop up in most of them. Building atop such a framework lets you benefit from hard-won experience gained across many projects.

redundant comments

from base64 import b64encode, b64decode  # Zum Obfuskieren der Daten
import csv  # All of the data is stored in CSV files
import re  # gex
...
import uuid  # To create unique ids

That first one is valuable as it explains the why rather than the how of the code, though the two lines above had already adequately explained that. The others should just be elided. Using the re module is bog standard -- you should assume your audience is familar with "batteries included" python libraries. Black and pep-8 won't interact well with your re#gex remark.

valid email regex

This is the sort of thing which is notoriously difficult to get right. I thank you for the link. However, the linked expression is very very different from your code.

_REGEX_VALID_EMAIL: Final = r"^\S+@\S+\.\S+$"

tl;dr: Your lenient expression which simply insists on @-sign plus . dot is much more suitable.

The linked expression ^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$ interacts poorly with addresses like [email protected], leser@bücher.de, [email protected], and hundreds of popular TLDs. Better that the UI will generously accept something that looks like a complete email has been entered, and defer the MX lookup and the SMTP "RCPT TO:" tests until later in the flow.

The three Final annotations are accurate, but not commonly used in annotated python code. No need to remove them, I simply find them slightly odd and redundant. The ALL_CAPS manifest constants already clearly communicate that we won't be changing this value. I suppose having mypy call it out is helpful? Ok. When I read those lines, it feels like JS or C++ const.

Recommend you elide the unused _UNSUCCESFUL_MATCH. And if you do want test a fullmatch(), simply ask if re.fullmatch(...):. If comparing specifically against the singleton None is important, we use if x is None: rather than if x == None:. Yeah, yeah, I know, it's just a weird python cultural thing, sorry about that, linters will help you get used to it.

raw string

_CSV_ACCOUNT: Final[str] = f"{_CSV_PATH}\\accounts.csv"

No. Please write it in this way:

_CSV_ACCOUNT: Final[str] = fr"{_CSV_PATH}\accounts.csv"

In general, if you have crazy characters that need escaping, prefer an r-string. And when you start writing any regex, even re.compile('hello'), always start out raw: re.compile(r'hello'). Sooner or later some maintenance engineer might insert special characters into an evolving regex, so better to begin on the right foot.

Finally, prefer to elide the redundant str annotation, as the string assignment to a constant already makes that clear to type checkers and to humans.

# Static variables inside a class for better readabilty

I respectfully disagree about the "why?", and would slightly rephrase:

# Static variables inside a class for reduced coupling

We're burying EMAIL and the rest down within the class so as not to pollute the module-level namespace with things that could be more local. Anyway, kudos for following that instinct, it's a good one.

There's nothing wrong with defining these constants, but maybe it's overkill? A string like "firstname" is not a magic constant if it comes from the requirements.

Benefits you may be reaping from these constants include

  • easier refactor if column is renamed? (doubtful, and your test suite will immediately reveal any straggler lines of code)
  • i18n (if we saw FIRSTNAME = 'vorname'; LASTNAME = 'nachname')
  • fewer typos, e.g. no confusion with "Firstname" or "FirstName"

Overall, I confess I'm skeptical these lines of code are worth it.

function names are lowercase

    def AsList() -> list[str]:

No. Pep-8 asked you nicely to call it as_list(). Similarly for init_from_dict(). Also, thank you for the helpful annotation.

Frankly, I would have coded up this constant function as

    def as_list() -> list[str]:
        return "id email password firstname lastname".split()

And in practice, I can often get away with obtaining such a list by letting csv or pandas read the header line of an existing .CSV file.

Calling such a class CSVHeader in three different modules suggests that outside modules shouldn't be importing this private class. Maybe you wanted to scope it down, burying it within the Account class?

extra type

    def InitFromDict(dictionary: csv.DictReader | dict[str, str]):

I have not attempted to run mypy on this. But are you sure that a DictReader is an acceptable input? I read that as a generator of dicts, rather than a dict.

Checking for short length is maybe a helpful diagnostic, but a failed de-reference will tell the same story soon enough, so consider eliding that check.

d makes sense, but prefer a nested def. The OP code creates an anonymous lambda, and then names it, uggh. GvR came this close to not even including the lambda keyword in the language, for fear of reading such code.

dataclass

The return Account( ... ) statement, and the __init__() ctor, are tedious and boring. Why can't we just return Account(**dictionary)? Why not use a namedtuple or @dataclass? It's no fun writing DAO classes such as this -- let the library support turn that crank for you!

Later we execute writer.writerow(dict(zip(CSVHeader.AsList(), values))), which just reinforces the need for dict(foo) to do the Right Thing.

CSV newlines

Yeah, sorry about that \r\n journey. Thank you for those valuable comments; do not delete them. Clearly they will benefit future maintainers, which could be you or someone else.

# region Private functions
# A csv.DictReader basically works like a list[dict[str, str]]
def _getdictreader_() -> csv.DictReader:

Elide that first comment line, as it is obvious; that's what the _ underscore prefix on _private() functions is for.

I kind of agree with second comment line, but I hope you understand that a lazy generator is very different from a list container. Consider phrasing the comment so it talks about list(some_reader), which pulls the values out of the generator. Note that you can iterate two or more times over a list, but only once over a generator, which is why we might store its results in a container.

Pep-8 asked you nicely to name it _getdictreader(). That trailing _ underscore doesn't carry meaning and it is confusing. Similarly for _obfuscate_text().

leaked resource

This is not great:

    accountfile_read = open(_CSV_ACCOUNT, "r", newline="")
    return csv.DictReader(accountfile_read, delimiter=",")

Prefer to habitually use a resource manager:

    with open(_CSV_ACCOUNT, "r", newline="") as accountfile_read:
        return csv.DictReader(accountfile_read, delimiter=",")

Now the filedescriptor will be properly close()'d. (Granted, this is more critical for the "w" case than for reading.)

type conflict

This makes little sense:

def _obfuscateText_(text: bytes) -> bytes:
    if type(text) is str:

You told me that caller promises to only pass in bytes, and then you immediately admit the possibility of passing in unicode instead?!? Raise fatal error if caller violated the rules. Or change the rules so they're more convenient to the caller. Also, are you even linting with mypy? Not sure how I would ever get my annotations correct without the occasional check.

utf-8

        text = bytes(text, encoding="unicode")

I confess I don't even know what that means. Maybe a locale-specific choice of UTF-16 vs UTF-32? In a random endianess? You're asking me to go look it up, but I don't notice its entry and grep finds many dozens of "unicode" occurences. Please just say "utf-8" if that's what you meant. BTW, the empty string means utf-8, as well.

Oh, later I see bytes(originalpassword, "unicode_escape"). I hope your test suite is verifying that even unusual characters will roundtrip successfully through that pair. If you're storing argon2id hashes, then even a 7-bit "ascii" codec should suffice, with no need for escaping.

error vs. None

I don't understand this Public API design choice:

def GetAccountFromEmail(email: str) -> Account | None:
    ...
    raise errors.AccountDoesNotExistError()

Doing return None would be a perfectly valid design decision. So is that raise. But please don't admit of the possibility of both, that's not clearly communicating your ideas to the Gentle Reader.

    reader: csv.DictReader = list(_getdictreader_())

That looks like a reader: list = ... to me. And then we could flesh out maybe reader: list[dict[str, str]].

tiny translation nit:
# the entire row empty is.
Looks like a leftover "ist" from "oder sogar die gesamte Zeile leer ist." Kudos on your translation effort, which overall is excellent, you are clearly a more than fluent Anglophone.

docstring

Thank you for this valuable # comment.

# Note: accountid == token stored in the cookie
def GetAccountFromToken(token: str) -> Account:

But since it's discussing the Public API this function defines, the remark belongs in a """docstring""" instead, please.

Pep-8 asked you to spell them get_account_from_token(), passwords_are_equal(), login_is_valid(), etc. And in many languages we prefer to start a predicate with is_, giving us is_login_valid().

    # accountfile_read = open(_CSV_ACCOUNT, "r", newline="")
    # reader: Iterable[dict] = csv.DictReader(accountfile_read, delimiter=",")

Please elide commented code. It won't be lost -- that's what git log is for.

implicit return None

Please don't do this:

def GetAccountFromToken( ... ) -> Account:
    ...
    for row in reader:
        if row.get( ...
            return Account.InitFromDict(row)

You promised that you definitely will return an Account. And sometimes you do. But sometimes you fall off the end of the function and implicitly return a wrong result.

Please tack on an explicit return None at the end of such functions. And then mypy will help you to adjust the signature to say ... -> Account | None:.

Alternatively, raise some fatal error, so we never return. You do a great job of doing this elsewhere in the codebase.

shebang

You had a very nice #! /usr/bin/python up top. But then there's no __main__ guard and no main line code being executed. Looks like you're not using the shebang. Recommend you

  • elide the shebang
  • chmod a-x accountmanager.py

Other modules will import it, and that is enough.

Oh, and when you do need a shebang, prefer to spell it this way:

#! /usr/bin/env python

That way it will respect the $PATH env var, and will use the python interpreter associated with your current venv or conda virtual environment, pulling in pip deps.

module docstring

#!/usr/bin/python
# * What is an Entry?
# An entry is what connects an account with an event.
# If, for example, a user registers for an event, then this is
# is saved as an entry/registration for this event

No shebang, as discussed above.

And the # discussion is lovely, but it belongs in a """docstring""" please.

Similarly for the eventmanager module.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.