Skip to content

Anna gst a4#60

Open
anna-gst wants to merge 5 commits into
mainfrom
anna-gst-A4
Open

Anna gst a4#60
anna-gst wants to merge 5 commits into
mainfrom
anna-gst-A4

Conversation

@anna-gst

@anna-gst anna-gst commented May 6, 2026

Copy link
Copy Markdown
Collaborator

Ready for review. Thx :)

@BeatriceHN

Copy link
Copy Markdown
Collaborator

Hey I reviewed your code and it works great!
The output is correct and I like all the additional flags you added as well as the error messages for flags being used incorrectly. I especially like what you did with the '.join(list). I haven't even considered the time complexity of small operations like concatenation of strings but you're absolutely right that str += str will take much longer for larger text. I definitely learned something new :)

I only have a small suggestion which is mainly for the readability of your code. It would be good if your args had individual names besides the flag name -o, -l etc.
When you wrote the error messages for the incorrectly used flags you referenced each flag with args.o or args.l. While it was easy to keep in mind what they represent, it can get confusing when you have to do this for many more args. I would recommend using some extra naming such as '-o', '--markov_order', then you can call it with args.markov_order. It's a small addition but it could help in the long run.

You really did great! And thanks for the new info!

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.

2 participants