Develop#898
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical issues that prevent the task from fully meeting the stated requirements. First, in app/main.py, the format string f"{start_letter}{i: 07d}" (around your brute-force loop) introduces a leading space, making the password 9 characters and non-numeric; this violates the requirement that passwords be exactly 8-digit numeric strings and likely prevents finding the correct hashes—please change it to f"{start_letter}{i:07d}". Second, inside check_range in app/main.py, you compute the hash by calling sha256 directly instead of using the required helper sha256_hash_str(password), which duplicates logic and does not follow the specification; you should replace the direct sha256 call with sha256_hash_str. Third, after gathering results from all processes and flattening them (e.g., flat_results), you do not verify that exactly 10 passwords were found; you should add a check like if len(flat_results) != 10: and handle the mismatch clearly (e.g., print an error or raise an exception) to satisfy the requirement of confirming that all 10 passwords were found.
As minor improvements, you might also assert that each found password is 8 characters and numeric to catch formatting regressions early. Overall, your multiprocessing approach and range partitioning are solid; once you fix these blocking issues, your solution will be much closer to the required behavior.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it now fully meets the functional and technical requirements of the task. You correctly brute-force the entire 8‑digit numeric password space using f"{start_letter}{i:07d}", ensuring all candidates are exactly 8 digits and numeric, and you validate this with an assertion on matched passwords. The hashing is properly centralized through the sha256_hash_str helper, and it is used consistently when checking candidates against PASSWORDS_TO_BRUTE_FORCE, which aligns with the specification. You also parallelize the brute-force search using multiprocessing.Pool and cpu_count() - 1, which satisfies the requirement to optimize performance. After collecting results, you ensure that exactly 10 passwords are found and printed, raising a ValueError if the count is different, which enforces correctness for the expected output.
One minor, non-blocking improvement would be to add a descriptive message to the ValueError, for example ValueError("Expected 10 passwords, found {len(flat_results)}"), to make debugging clearer if something ever goes wrong. Overall, your solution is correct, robust against formatting regressions, and aligned with all checklist items. Nicely done bringing everything in line with the earlier review feedback.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Uh oh!
There was an error while loading. Please reload this page.