prmr / Creco

Recommendation System for Consumer Products
Apache License 2.0
6 stars 2 forks source link

Product Ranking #46

Closed prmr closed 10 years ago

prmr commented 10 years ago

Unhook the product search and rank the products alphabetically. Keep the product search code around.

mangalagb commented 10 years ago

Merged changes into master.

forgues commented 10 years ago

I had a look at the changes you made, and I have a few recommendations. Let me know if you agree with them or not.

  1. I noticed you removed the search functionality of ProductSearch and replaced it with alphabetical sorting. Since the class now simply sorts a list of products rather than doing search, I recommend renaming ProductSearch and moving it out of the search package.
  2. The method that does the alphabetical sorting, public List<ScoredProduct> returnProductsAlphabetically(String pQueryString, String pCategoryID) still takes a query string (pQueryString). However, the method never uses this parameter. You should probably remove the parameter from the method if it's never used.
  3. The ScoredProduct class no longer has a "score", so I'm not sure it's a very useful object as it stands right now. It's essentially a wrapper around a product. If other people still need a ScoredProduct, we should keep the score field in. But if no one needs the score, then we might as well remove the class entirely.
  4. I get failing unit tests from TestProductSearch, which is to be expected since the product search functionality was removed. You could remove the test class entirely and add unit tests for the alphabetical sorting instead.

If anyone has an opinion on keeping or removing the ScoredProduct object, please let us know!

prmr commented 10 years ago

If ScoredProduct is not used, it should indeed be removed.

mangalagb commented 10 years ago

I kept scored product as a wrapper around as it was being used by attribute extractor as well. Removing that will require changes to attribute extractor and its tests as well. But I can surely change that.

asutcl commented 10 years ago

It would be safe to remove ScoredProduct. All methods that use ScoredProduct have been deprecated since we don't score attributes based on a subset of items anymore. You might only need to change the constructor calls to AttributeExtractor to only pass the category.

mangalagb commented 10 years ago

Ok then. I'l remove ScoredProduct entirely. Since product search is no longer required, would it be more sensible to add the returnProductsAlphabetically method to CategorySearch directly ?

forgues commented 10 years ago

CategorySearch returns categories rather than products, and I think it would be best to keep searching and sorting separate anyways. You could put the sorting method in a new static class, and we could eventually add more methods to this class (e.g. sorting by price).

prmr commented 10 years ago

There are three failed tests in the master branch for TestProductSearch. 18ffa552cc000ac542b968d974bc82aae2df8c0c

mangalagb commented 10 years ago

As of now, I have removed the failing test cases from TestProductSearch But I have not removed the ScoredProduct wrapper. When I attempt to remove it entirely and just return a list of products alphabetically sorted, I run into a lot of errors especially in RankedFeaturesProducts which contain methods that are not yet deprecated. Mariam can you help me here?

MariamN commented 10 years ago

When changing ScoredProduct to Product I get a lot of errors about conflicts between data.Product class and logic.search.Product class. Although they are in different packages but still eclipse is giving me errors. I'm a bit confused which one to use now, and why they have the same name?

asutcl commented 10 years ago

The idea of removing ScoredProduct was to use the data.Product since ScoredProduct had no additional features/capabilities.

MariamN commented 10 years ago

@mangalagb I updated RankedFeaturesProduts it now uses only Product class. I just pushed into master.

mangalagb commented 10 years ago

Made the following changes 1) Removed unused parameter querystring in ProductSort 2) No more ScoredProduct used.