Skip to content

Conversation

@yiwork
Copy link
Owner

@yiwork yiwork commented Apr 13, 2016

No description provided.

roman.py Outdated
sys.stdout.write('%s' % ROMAN_DIGIT[base] * quotient)

get_roman_digit(remainder, base/10)
get_ROMAN_DIGIT(remainder, base/10)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you overdid the search and replace here

@jonmcoe
Copy link

jonmcoe commented Apr 13, 2016

no output at all for inputs under 10.

➜  codesample git:(master) python roman.py 1

➜  codesample git:(master) python roman.py 2

➜  codesample git:(master) python roman.py 3

➜  codesample git:(master) python roman.py 4

➜  codesample git:(master) python roman.py 5

➜  codesample git:(master) python roman.py 6

➜  codesample git:(master) python roman.py 7

➜  codesample git:(master) python roman.py 8

➜  codesample git:(master) python roman.py 9

➜  codesample git:(master) python roman.py 10
X

not sure if the problem is with the get_largest_base or get_roman_digit function. i didn't really dig. unittests will help you catch stuff like this in the future

@jonmcoe
Copy link

jonmcoe commented Apr 13, 2016

don't really like how the get_roman_digit prints as it goes. you should probably have it return the value and print it in main

@yiwork
Copy link
Owner Author

yiwork commented Apr 13, 2016

Suggest what unit tests I should do. Not sure how it could be done when the program is recursive.

@jonmcoe
Copy link

jonmcoe commented Apr 13, 2016

Once you switch the central function to return value instead of print, you
just test equality

Assert that get_roman_digit(x) == y

For a comprehensive set of input/output
On Apr 13, 2016 13:56, "yiwork" notifications@github.com wrote:

Suggest what unit tests I should do. Not sure how it could be done when
the program is recursive.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#1 (comment)

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