Skip to content

Instantly share code, notes, and snippets.

@philsof

philsof/code-review-roman-numerals-challenge-branch-kbjk.md

Last active January 26, 2016 06:17
Show Gist options
  • Save philsof/0ca485ef5003e677b910 to your computer and use it in GitHub Desktop.
Save philsof/0ca485ef5003e677b910 to your computer and use it in GitHub Desktop.
Download ZIP
code review for kbjk branch on roman-numerals-challenge
Raw
code-review-roman-numerals-challenge-branch-kbjk.md

Your code works, and is easy to follow. Nice! Here are some notes from me:

  • Be sure you use names that clearly convey what your objects are/are doing. Specifically: On line 2 of your code, you declare a variable named new_array. That name is way too vague. From its name, all I know is that it is an array. How about something like roman_num_array? It needs to be something that tells me what the variable represents.
  • Your approach to this challenge is sound. You realized the need to start with the largest Roman numeral amount (1000) and work your way down, and decrease your Arabic number along the way. But there are better ways that eliminates the need to use any while loops (or any loops at all). Hint: check out these methods: div and divmod. How could you refactor your code in such a way that no loops are needed? There is a way. You can do it!
  • Your code has good indentation, which makes it easy for me to read. Thank you!
  • After refactoring, if you have time, write more tests for all different types of numbers/Roman numerals, and implement modern Roman numeral handling.

Good work!

Any questions let me know.

-Phil

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