Tubular Tulips #13
Tubular Tulips #13
Conversation
Set up project
Add Pyodide type stubs
feat: add aes implementation
feat: add main flow for the app
Implementation of lock designs
Create README.md
include link to presentation video
camcaswell
left a comment
camcaswell
left a comment
There was a problem hiding this comment.
Hello, congrats on completing the Code Jam and making it into the top 10! Here are my general thoughts about your project after reviewing it:
Your README is good. It accurately and succinctly explains what the project is and I was able to install and run it easily.
Most of your commits are good quality, and the handful that aren't are excusable given the semi-chaotic nature of the jam.
Your code was generally clean and idiomatic.
Structurally, the only problem I saw was the one I commented on. The factoring out of the Methods class from the main App class left me momentarily confused about its purpose and resulted in the on_key_received callback being awkwardly passed around.
As for the documentation, I thought some your variable names were good but some could use more specificity. E.g. it may be obvious what a variable called data refers to in the context of decryption, but less so when it's an argument to a function updating the frontend. It's important to give your variables names that are helpful to the reader in whatever context they appear.
Also docstrings! Docstrings are one honking great idea -- let's do more of those!
On the whole, reviewing your code was pretty smooth and I liked your project. My one complaint was that I couldn't spin the safe wheel with momentum :p. Feel free to post any follow-up questions or comments here, and good luck!
|
|
||
| if self._container is not None: | ||
| for method in methods: | ||
| if method.byte == self._container.method: |
There was a problem hiding this comment.
Defining a Method enum would clarify this code and replace methods.py:28-37
| ] | ||
|
|
||
|
|
||
| class Methods: |
There was a problem hiding this comment.
This class raises 2 red flags for me: its name is a plural and it's a singleton.
I believe its functionality should be included within the App class. I think it's fine if that makes the class representing the base of the app be a bt longer, but if you strongly disagree I think this class should be renamed to something like MethodsManager to make clear that it is not intended to represent a singular Method.
| document.title = "Super Duper Encryption Tool" | ||
| document.body.innerHTML = await fetch_text("/ui.html") | ||
|
|
||
| self._data: bytes | None = None |
There was a problem hiding this comment.
This could probably be a more descriptive name
| offset += filename_length | ||
|
|
||
| data_hash = struct.unpack("32s", raw[offset : offset + 32])[0] | ||
| offset += 32 |
There was a problem hiding this comment.
I like to leave short comments explaining small adjustments without obvious motivation
| from cj12.dom import add_event_listener, elem_by_id | ||
| from cj12.methods import KeyReceiveCallback | ||
|
|
||
| # NOTES |
There was a problem hiding this comment.
I recommend maintaining notes about your project in a separate notes file. It'll be easier to edit your notes without comitting those changes in git, easier to edit your code without your notes getting in the way, you won't have to decide where to move your notes if you refactor this file, etc.
No description provided.