coderly / teamplaybook-ember

0 stars 0 forks source link

Refactored url-info library to make it less confusing #9

Closed begedin closed 9 years ago

begedin commented 9 years ago

Self explanatory.

venkatd commented 9 years ago

Looks good. Take a look at the comments for suggestions.

begedin commented 9 years ago

@venkatd: If I'm moving it into external files, then I think it might be a good idea to restructure it a bit.

The only question is, do I extract the two methods into separate file (named after the method), or in a utils/url file, where both methods are exported individually (instead of using a default)?

I'm slightly leaning towards the second option (might be slightly easier to manage), but both could be argued for.

venkatd commented 9 years ago

I like the second option too.

begedin commented 9 years ago

@venkatd I did the extraction to a "url-utils" library.

I decided to keep the mapping method inside url-info, since that part of the behavior is specific to that library and not something we should generally apply. I also renamed the two methods to something more descriptive, to avoid confusion.

Finally, I added a bunch of unit tests for both libraries. I don't think it's worth it to start adding integration tests, since we don't know what the interface is going to be yet, but I should have written some unit tests sooner.

venkatd commented 9 years ago

Looks great. Some minor corrections.

begedin commented 9 years ago

Made the changes, so I'm merging now.