Refactor Data API - Githubissues

enewe101 commented 10 years ago

Remove all deprecated methods
Make internal methods (such as Categorys AttributeStat getters) package-private
Cleanup CategoryBuilder and move its functionality into CategoryNode
The code of `CategoryBuilder.intersectRatings' looks suspicious.

prmr commented 10 years ago

I'm splitting this into two issues: Refactoring and testing.

enewe101 commented 10 years ago

Question about item 3 in the description "Cleanup CategoryBuilder and move its functionality into CategoryNode:

I'm not clear on what this entails exactly. Obviously not just renaming CategoryBuilder as CategoryNode!

I think it's something along the lines of changing the relationship between Category and CategoryBuilder -- i.e. not having a call to Category's constructor right inside CategoryBuilder, but rather, in build logic outside that scope. Is there anything else that should be eliminated from the functionality in CategoryBuilder as this functionality is moved to CategoryNode?

I'll do my best here but any high-level pointers will help me produce something more in line with what you have in mind!

prmr commented 10 years ago

I don't have a clear answer. I think it's just a question of clarifying the separation of concerns between the two classes to make testing easier. It looks to me like the design of CategoryBuilder can be improved a lot, too (e.g., by finding a cleaner (non-flag) way to do the intersection).

enewe101 commented 10 years ago

So far:

All traces of ratings and specs removed from Category and CategoryBuilder
AttributeStat has been entirely removed from the application - only the minimal data about the attribute membership of CategoryBuilders needed for calculateJaccard() is kept now.
As a result, a few complicated methods relating to merging sets of attributes in CategoryBuilder have been simplified/deleted- Nearly all CategoryBuilder functionality was renamed to CategoryNode. The build logic was moved to the CRData constructor.

enewe101 commented 10 years ago

Also:

Products.getCategory() used to be return a CategoryBuilder -- now it returns a Category.

The changes on this branch make some of the CRData testing obsolete. I'm going to tackle the issue of writing tests next.

I think this issue is ready to close. I also think I should merge with master. (All tests passing). If anyone has an issue with that let me know. Otherwise I'll be merging soon.

enewe101 commented 10 years ago

merged

mangalagb commented 10 years ago

The master branch seems to be broken. I am unable to run the application or the tests.

enewe101 commented 10 years ago

It's working fine for me. I ran the tests and did manual testing of the app before pushing. Just double checked again, and it's working. What kind of error are you getting?

prmr commented 10 years ago

Works fine for me too.

mangalagb commented 10 years ago

This is what it says "Exception in thread "main" org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'siteController': Injection of autowired dependencies failed;"

mangalagb commented 10 years ago

My local copy of master seems to have been corrupted. I deleted it and pulled it freshly and all works fine now. Sorry for the trouble.

enewe101 commented 10 years ago

Weird eh? It's happened to me periodically too...

prmr / Creco

Refactor Data API #63