Skip to content

Fixed some bugs and made code harder to follow #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sarudak
Copy link

@sarudak sarudak commented Mar 23, 2015

You original version of gilded rose was great but it didn't fulfill all the requirements.
-Items were not prevented from having their quality go below 0
-Items were not always prevented from having their quality go above 50

Additionally not exactly a bug but significantly different from the original is that all item names not already in the list had strange behavior quality was unchanged (like sulfuras) unless expired where they acted like normal items and their sell in decreased normally. I fixed all these issues. I wrote tests to verify the issues and the fixes but obviously including them would be against the spirit of the exercise. :)

@trptcolin
Copy link

Yeah, I think your fixes make sense, but consider that this repo is a teaching exercise, where you start w/ some really bad legacy code and fix it. In the real world, it turns out that code without tests is often also broken in terms of features ("How did this ever work?" is a common question when code looks like this.)

I know this has confused others in the past as well, though. I'd personally lean towards clearer documentation (maybe a hint like "We've heard that there might be some bugs in the system, so hopefully you can fix those"?) rather than fixing the bugs in the starting-point code.

@sarudak
Copy link
Author

sarudak commented May 20, 2015

That's true that legacy code is often has oddities in it and occasionally errors. In my experience when legacy code that's been a round for a while has differences from the stated requirements of the system more often than not it's the legacy code that's correct and it's some edge case or exception that the user didn't remember about or didn't know was important.

I only brought them up because the original gilded rose did not have the defects and I felt like introducing defects intentionally adds too much to a complex kata that already involves writing a test suite for existing code from scratch, refactoring an ugly piece of code, and adding a new feature. Additionally I felt like the clojure version was too easy to follow relative to the original C# version (which is partly because clojure just won't let you do certain terrible things) and the additions make the code harder to parse.

In any case it's up to you. Just thought I'd share the changes i made.

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