Itinerant Iterators

[janine9vn]

No description provided.

[ichard26]

Hello, I'm Richard and I'm the assigned reviewer for your project! Congratulations on submitting a top 10 project.

My review is structured in five parts:

1. Use of Git and GitHub + Project infrastructure (GHA and other developer tools)
2. Code review for the application
3. Code review for the server
4. Code review for the tests
5. Final comments (spoiler: it's going be pretty short, the juicy details will live in the four other parts 🙂)

This review is the first part. And before I get into it, remember, if you have any questions or want to discuss my feedback, please let me know!

Use of Git and GitHub

The Git commit history is decent. I like how most commits are "atomic" and are reasonably sized and focused. However, I would like to see more context on commits that have vaguer titles. For example, smileyface12349/itinerant-iterators@ca0eb1c 's commit message doesn't explain what was broken about the obfuscator... which makes it hard to tell what the commit does at a glance.

Also, don't forget to squash or use interactive rebase to clean up local commits before pushing to avoid situations like this one:

I'm happy to see GitHub Issues used to keep track of work and discuss tasks. Keep that up! It's a skill you'll need a lot. If you want to take it further, I'd suggest using Pull Requests to encourage code review before a change is merged.

Further reading:

• https://cbea.ms/git-commit/
• https://about.gitlab.com/blog/2020/11/23/keep-git-history-clean-with-interactive-rebase/
• https://stackoverflow.com/questions/5308816/how-can-i-merge-multiple-commits-onto-another-branch-as-a-single-squashed-commit
• https://docs.github.com/en/get-started/quickstart/github-flow

Project set up & infrastructure

The way the project is set up makes sense. Tooling like Black and pre-commit are appropriately used to maintain a consistent code style. Great job on using a GitHub Actions workflow to automatically run tests on each push.

I think the user experience would improve if the application was packaged into an installable Python package, but I'll discuss this further in Part 2.

[ichard26]

Make sure when you're copying and pasting any sort of code that you make the necessary modifications so it fits the context. It's too easy to end up with confusing comments like this :P

[ichard26]

I love to see workflow_dispatch to be used here. It's a great feature.

[ichard26]

We don't hold the copyright to your project's code, don't forget to change this next year if you're participating again :)

[ichard26]

I'm pleased to see the license information was maintained. You don't see that too often these days!

[ichard26]

You don't need this folder at all. 🙂

[ichard26]

Pin the version of Black used to at least a major revision so you don't face sudden formatting changes when the new year rolls around. For more information, see Black's stability policy.

[ichard26]

I love the use of a task runner. It makes getting up and running with a new project so much easier and faster.

Although, while just is a perfectly capable task runner, I would suggest looking into Tox, Nox, or even Hatch's own task runner. They have automatic environment provisioning (ie. setting up virtual environments and installing dependencies as necessary) that is tailored to Python's ecosystem. They may be more intuitive and provide a more streamlined experience compared to just.

[ichard26]

(Part. 2: Code Review of the application)

Code quality & structure

I appreciate and encourage the use of type annotations! It helps massively with the readability and maintainability of the code. However, there are certain places where type annotations are incomplete or lacking. Some APIs expect callables or complex data structures and it'd be better to thoroughly type them.

On top of that, I'd highly suggest using type aliases. Not only do they help with reducing repetition, but they give names to often complex types. For example, a Bound type alias for tuple[int, int, int, int] would be amazing!

For example, here's the beginning of the Lsb class but with the feedback above incorporated. Notice how the type annotations further explain what the code does at a glance, improving readability ("what does pixels_generator do?" ... "ah I know, it takes a list of Pixels and yields pixels in a certain order").

from collections.abc import Iterable
from typing import Callable

# This type alias should be defined somewhere else in practice,
# but for sake of the example, it'll live here.
Pixel = tuple[int, int, int]

class Lsb(Steganography):
    def __init__(
        self, pixels_generator: Callable[[list[Pixel], bytes], Iterable[Pixel]] = pixels_top_left
     ): ...

Finally, I forgot to mention this earlier, but I'd firmly encourage using a type checker (eg. mypy or pyright) to check and enforce types. Using type annotations is one thing, but having machine checked type annotations (and by extension, type documentation) is even better!

The application code is generally well organised. I like how the obfuscation and steganography code are sectioned off in their own subpackages. Most functions and classes are well designed, naturally documenting and delineating sections of logic (in a way that follows DRY and the SRP).

Certain functions and modules could be renamed and moved, but there isn't anything truly terrible. I do think some portions are overengineered and can be simplified (eg. some classes could be replaced with a module with a set of functions). Overall, it was generally easy to read the code and figure out the logic flow across the various modules.

Anyhow, that concludes my overall comments. Please see the comments below for specific feedback.

Code documentation

I'm pleased to see docstrings. They're consistently used and complete with all parameters and return values documented. However, many arguments and return values could be further specified, and some docstrings only state the obvious. Overall, they are decent and I would rather have them than not, but there is some room to improve.

More importantly, the codebase would strongly benefit from more comments. There are some areas that implement very involved logic and are challenging to read and follow. Adding comments that explain what is going on, especially the why would be a good next step for improving readability. Cases where this problem was particularly bad are noted in this review's comments.

The README is short, but it's clear and and does a good job at highlighting the features and the project can be used. I'm glad examples and installation docs are included. Kudos!

User interface (CLI/GUI)

Attention: I have never used PySimpleGUI so I was unable to make specific comments on its use. I tried my best to provide feedback on the other parts though.

The CLI and GUI are well designed and work well, but I would suggest a different way to handle the GUI/CLI distinction. Since the command line interface is (according to the README) the primary way the application is meant to be used, instead of implementing a "launcher"-type CLI, I would suggest placing the GUI start up code in a ui.py module guarded by if __name__ == "__main__":.

This simplifies the whole CLI, removing a layer of complexity. Instead of commands like python -m app cli watermark decode image.png, an user can drop the (clunky) cli part and write python -m app watermark decode image.png. (All while starting the GUI is still easy: python -m app.ui.)

In addition, if the application was packaged using say, flit, so it could be installed using pip, project scripts could be defined so once installed, the user could run (pretend the application is called "imgedit") imgedit or imgedit-gui in their terminal to launch the CLI/GUI for a smooth terminal UX.

Finally, to bring it back to the CLI, it could do with some better documentation. Command docstrings should include rich descriptions and examples and most options should have a description too. I left a comment on app/cli.py that goes into further detail. (TL;DR, it shows when --help is passed`)

[ichard26]

I don't have a preference between relative and absolute imports, but being consistent is important. This breaks running the application from a different directory that isn't the repository root. I'd suggest using relative imports as those seem to be a better fit (the project isn't installable):

from .cli import main

[ichard26]

Avoid repeating the argument and return type annotations in the docstring. An IDE should be able to use both to provide rich help. Down the road, it's very likely the docstring types will be out of date with the actual type annotations.

(This applies to the rest of the project, but this was the first example of it.)

[ichard26]

This is what I mean by unclear docstrings. It's good that they exist, but it's no good if they do little to help the reader with what the function does. How about this?

Suggested change

	"""Set the bit as position on the value.
	Args:
	value (int): Value to set bit on
	bit (int): Bit position to set
	Returns:
	int: value with the bit at position `bit` set
	"""
	"""Set a bit in a byte to one.
	Args:
	value: Byte to set bit on
	bit: Bit index to set
	Returns:
	int: Value with the bit at :bit: location set.
	"""

[ichard26]

It's great that the bit manipulation functions are located in their own module, but this should probably live in app.steganography since the obfuscation code makes no use of this code.

[ichard26]

Excellent usage of Click. Command groups, parameter types and other Click features are effectively used to compose a clear CLI. The application's UI could still use some work though. Please see feedback written above.

[ichard26]

The application's GUI works very nicely (barring a few bugs). Well done on using PySimpleGUI!

A note on code structure, I would suggest moving the UI layout definitions to be module level constants to avoid a level of indentation/nesting. I would also like to see the various "actions" to be handled by individual functions, in lieu of having all of the UI code in a single massive function.

[ichard26]

Enums are underrated IMO, it's great to see them used here.

[ichard26]

So the reason why applying watermark doesn't seem to work is that this code uses the original image (path). The image variable is correctly updated after applying the watermark (so saving and then using the CLI to decode works), but that's ignored here.

This is a bit annoying to fix because steganography.Image doesn't accept a Pillow Image instance. Ideally, you would be able to pass the image variable so any updates made to it would be picked up. This is one of the risks of implementing class-based abstractions, they work great if they fit the needs, but are an absolute pain if they don't quite match. Using a set of functions is generally more flexible. But, updating the steganography.Image class to accommodate this usecase wouldn't be a bad option either.

[ichard26]

I'm impressed to see the terminating underscore convention for names that would (otherwise) shadow a built-in name, kudos!

[ichard26]

Solid use of functools.cached_propery!

[ichard26]

(Part. 3: Code Review of the server)

It's neat how your project incorporates Google Cloud Vision. I like how regex is used here to build a really cool feature (regex-based obfuscation). However, the server-side code needs improvement. As-is, it's hard to read and is dubiously written. Other than my specific comments, my two big suggestions are to:

• Break out logical sections into individual functions. This will encourage code reuse (and there's a fair of fairly repetitive code here) and cleaner variable use (several variables have multiple purposes which is bad). Smaller functions tend to be also easier to read since there's just less going on.¹
• Write more comments, I really mean it :)

Footnotes

1. It's a balancing act though. Sometimes too many functions is a bad thing. It's like blank lines in an essay. You shouldn't write your whole essay without a single blank line, but you also don't want to have a blank line for every 4 sentences. A general rule of thumb is each function should implement a single "step" of your program's overall logic (the little details usually don't matter/warrant their own tiny functions). ↩

[ichard26]

I would suggest reading the Google API credentials using a different method. Grabbing everything in os.environ and then putting them in a "temporary"¹ file is not recommendable. Either pass the credentials directly to the client from certain environment variables or require the GOOGLE_APPLICATION_CREDENTIALS envvar to be set to a valid credentials file at start up.

Footnotes

1. I'm not sure what tempfile does exactly on shutdown, but there's a decent chance the temporary file is never cleaned up as it's not closed anywhere as far as I can tell. ↩

[ichard26]

Very cool to see itertools be used, but this is a bit more idiomatic.

Suggested change

	text = list(itertools.chain(*matches))
	text = list(itertools.chain.from_iterable(matches))

[ichard26]

There's only one minor difference between the two branches, merge them.

Suggested change

	image_chars = []
	for page in document.pages:
	for block in page.blocks:
	for paragraph in block.paragraphs:
	for word in paragraph.words:
	for symbol in word.symbols:
	if not regex:
	image_chars.append([
	symbol.text.lower(),
	(symbol.bounding_box.vertices[0].x,
	symbol.bounding_box.vertices[0].y,
	symbol.bounding_box.vertices[2].x,
	symbol.bounding_box.vertices[2].y)
	])
	else:
	image_chars.append([
	symbol.text,
	(symbol.bounding_box.vertices[0].x,
	symbol.bounding_box.vertices[0].y,
	symbol.bounding_box.vertices[2].x,
	symbol.bounding_box.vertices[2].y)
	])
	image_chars = []
	for page in document.pages:
	for block in page.blocks:
	for paragraph in block.paragraphs:
	for word in paragraph.words:
	for symbol in word.symbols:
	image_chars.append([
	symbol.text if regex else symbol.text.lower(),
	(symbol.bounding_box.vertices[0].x,
	symbol.bounding_box.vertices[0].y,
	symbol.bounding_box.vertices[2].x,
	symbol.bounding_box.vertices[2].y)
	]

[ichard26]

Repurposing variables, especially parameter variables, tends to make it harder to keep track of what's happening and how data is flowing/being modified. Modifying a parameter variable is okay, but repurposing it for something else is almost never a good idea. Keep the different pieces of data separate.

[ichard26]

This is frankly hard to follow. I would suggest adding comments and changing variable names to be more descriptive. With simple logic, it's possible to get away with bad variable names, but once there's some complexity, good names and comments are crucial!

From what I can glean, this seems to go through each character in the image (as detected by GCV) and checks whether it matches a character in the text to filter. If there's a match, then the next character in the image is checked to see whether it matches the next character of the filtered text (while skipping spaces) and so on...

This is a reasonably efficient and straightforward algorithm! (despite the readability issues with the implementation)

(A good start would be including the description I just provided or your own in a comment as a heads-up/aid to the reader.)

For example, this is a rewrite of this code with better names and with a different approach to keeping track of what characters of the text we're looking for have been found:

    if not regex:
        search_characters = list(text.lower().replace(" ", ""))
        remaining_search_characters = search_characters.copy()
        current_matching_bounds = []
        for image_char, image_char_bound in image_chars:
            # Grab the next character we're still looking for.
            try:
                search_char = remaining_search_characters.pop(0)
            except IndexError:
                # No more characters are left to check. Restart.
                remaining_search_characters = search_characters.copy()
                current_matching_bounds.clear()

            if search_char == image_char.lower():
                current_matching_bounds.append(image_char_bound)
                if len(remaining_search_characters) == 0:
                    bounds.extend(current_matching_bounds)
                    remaining_search_characters = search_characters.copy()
                    current_matching_bounds.clear()
            else:
                # A character did not match, bail and reset the search attempt
                remaining_search_characters = search_characters.copy()
                current_matching_bounds.clear()

I did not test this, so this may contain bugs and may not produce the exact same results, but the idea behind my feedback is there.

[ichard26]

(Part 4. Code review of the tests)

Overall well done with the tests! It's great to see tests (and well designed tests!) included in the project, they're an excellent way to up and maintain code quality even in the face of rapid changes. I just have a few suggestions to make the code quality even better.

[ichard26]

This is great! I love how a testing format is clearly being followed and how the API contract of byteutils.iter_bits() is being tested.

[ichard26]

How about this? (the assert was also supposed to be per-loop, right?)

Suggested change

	for case in cases:
	got = byteutils.set_bit(case[0], case[1])
	assert got == case[2]
	for value, position, expected in cases:
	got = byteutils.set_bit(value, position)
	assert got == expected

[ichard26]

The same unpacking assignment trick used above works here too.

[ichard26]

Solid integration test. Could be made cleaner though.

Suggested change

	def test_lsb_encode_decode():
	message = "secret message"
	lsb = Lsb()
	f = tempfile.NamedTemporaryFile("w", delete=True, suffix=".png")
	encode(message, Path("tests/data/python-logo.jpg"), Path(f.name), lsb)
	got = decode(Path(f.name), lsb)
	f.close()
	assert message == got
	def test_lsb_encode_decode():
	message = "secret message"
	with tempfile.NamedTemporaryFile("w", delete=True, suffix=".png") as _f:
	temporary_file = Path(_f)
	encode(message, Path("tests/data/python-logo.jpg"), temporary_file, algorithm=Lsb())
	decoded_message = decode(temporary_file, lsb)
	assert message == decoded_message

If you don't understand what the with statement is, here's a Real Python link, but anyway, it just manages the cleanup of the temporary file. Once execution leaves the with block, the temporary file is closed (and deleted).

[ichard26]

This seem like a great candidate for parametrized tests.

@pytest.mark.parametrize(
    "obfuscator", [ColourBox("black"), ColourBox((0, 0, 0))], ids=["by_name", "by_tuple"]
)
def test_obfuscate_colour_box(obfuscator):
    # arrange
    # create a new blank image with all white pixels
    img = Image.new("RGB", (100, 100), (255, 255, 255))

    # act
    obfuscator.hide((25, 25, 75, 75), img)

    # assert
    all_pixels = list(img.getdata())
    white = 0
    black = 0
    for pixel in all_pixels:
        if pixel == (0, 0, 0):
            black += 1
        elif pixel == (255, 255, 255):
            white += 1
        else:
            raise Exception(
                f"obfuscated image contained pixel of unexpected colour: {pixel}"
            )

    assert img.size == (100, 100)
    assert black == 2500
    assert white == 7500

Also, perhaps the output could be checked against a known-good image file? That's real nickpicky though :p

[ichard26]

(Part 5: Final comments 🎉)

Your team's project is on the smaller side, but it felt complete and generally polished (barring a few bugs with the GUI). I enjoyed using your application during this code jam! The code quality is decent overall. There were many things I liked (eg. tests, the use of typing.Protocol, the use of Click), but like almost anything in life, there was also room to improve. Summarizing from my lengthy review, the main suggestions are:

• Use better names for variables and classes
• Use functions to organise code into logical sections¹
• Adopt a type checker to enforce quality, reliable, and complete type annotations
• And finally, more comments please! I sound like a broken record on this point, but I really mean it..

Please don't take my review as an attack on your abilities — despite my many comments, I'm pleased with your submission — we just want to provide y'all with constructive and detailed feedback so you can all improve. 🦆

If you have any questions or want to discuss my feedback, please reach out! Also, I'm far from being a perfect developer, so if you disagree with some of my feedback, that's okay! Take what you want from this review.

Congratulations on making Top 10. Good luck for the livestream!

Footnotes

1. this basically only applies to the server code! ↩