search

prmr / Creco

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

SiteController Desing Questions #108

Closed prmr closed 10 years ago

prmr commented 10 years ago

A few questions to make sure everything is alright.

1) In SiteController.init(), why is myform set as a model object? This does not seem to be used anywhere and this function could just act as a URL rewriter.

2) Why is FeatureView session-scoped?

3) Why do we still needs the View objects? It looks like these are just remapped from domain objects in the facade. It is not possible to bind directly to the domain objects, e.g., ScoredAttribute and a reincarnated version of our previous ScoredProduct class?

4) Why is this method named searchRankedFeaturesProductsPOST? What's the deal with POST?

5) Why is this method named updateCurrentFeatureList? Does it have a side effect? I couldn't see that it did.

5) The large if statement at the end of sendCurrentFeatureList looks like it's creating a JSON string. First, this should be extracted into a separate private method. More importantly, we can use Gson to serialize as well as deserialize JSON. Can't this be done here?

ceipher commented 10 years ago

3) View Object is designed and used on a Software engineering purpose, it's put there for future potential replacement of backend object, e.g. Product class structure. So our 3rd-party client needs only modify the converter then. For now, we don't need it. 4) Yes, it should be removed. It was used for fixing a previous bug. Now we don't have post/get issue anymore 5) & 6) @MariamN

ceipher commented 10 years ago

1) it is not used anywhere. not sure who added this. I think we can remove this too. 2) Yes we can remove the session annotation for class, we are not using session concept for feature anymore

MariamN commented 10 years ago

5) Back in the days, it was used to update the values of the selected attributes according to the user input. We can refactor this now.

MariamN commented 10 years ago

points 5 & 6 addressed in the latest commit.

priyasidhaye commented 10 years ago

The form in init() is used in RankExplanations, so can't remove it. I tried and it gave errors while selecting/deselecting attributes. The other things I changed in the earlier commits.

priyasidhaye commented 10 years ago

All the changes in this are merged into master now, so I'll go ahead and close this one.