Skip to content

eweindorfer A4#52

Open
eweindorfer wants to merge 2 commits into
mainfrom
eweindorfer-A4
Open

eweindorfer A4#52
eweindorfer wants to merge 2 commits into
mainfrom
eweindorfer-A4

Conversation

@eweindorfer

Copy link
Copy Markdown
Collaborator

Ready for review!

@kralina123

Copy link
Copy Markdown
Collaborator

The code seems to work well and supports the required options like -o, -w and -s, and even more. I really appreaciate the clear structure of your code. This makes the code easy to follow. The README file is very clear as well.

One small thing to reconsider is the removal of punctuation in character-based mode. The assignment explicitly mentions that punctuation may remain in word mode, but it does not require removing punctuation in character mode. So keeping the original characters might be closer to the specification.

One possible improvement could be memory efficiency. The current dictionary approach is simple and easy to understand, but it could become quite memory-heavy for larger texts.

One edge case that could be considered is what happens if the chosen order k is larger than the number of available tokens in the input text.

Overall, a very nice implementation! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants