Refactor SiteController

[prmr]

We also need an owner in charge of SearchController who can do some nice design work on the front end. Currently too much logic is implemented in the controller, which should be moved to the logic layer.

It also seems to be there should be a better way to use databindings to avoid coverting our domain objects to view objects.

There should be an owner for class SiteController

RankedFeatureProducts also needs to be refactored.

[prmr]

I'm merging #74 into this issue.

[prmr]

I managed to get a basic prototype of the minimalist design going. Stuff left to do:

• Map directly to Product objects in the view and get rid of the view wrapper classes
• Move the logic to the ServiceFacade and underlying components, and get rid of RankedFeatureProducts
• Get rid of the instance fields.

This should be coordinated with #68 and the various issues touching the logic component. @priyasidhaye , @ceipher, when can you pick up on this?

[forgues]

RankedFeatureProducts is doing a lot of work (e.g. computing attribute correlations and directions) which is already being done in the AttributeExtractor. I will replace most of the logic in RankedFeaturesProducts by calls to AttributeExtractor. Depending on the size/complexity that remains, we can move it all to ServiceFacade or move it into some smaller component (ProductRanker?) which will be called by the ServiceFacade. I'm coordinating this with @asutcl who is converting AttributeExtractor into a bean first.

[prmr]

Very timely. Once this is done, it will be much easier to wrap up the SiteController. The question is whether to branch off of master or Issue0075-mpr.

[forgues]

I think @asutcl will be merging his change into master. Then I can simplify RankedFeatureProducts in master branch as well. Those two changes shouldn't cause conflicts.

Once those changes are done, and depending on if Issue0075-mpr still has unmerged changes, we can decide where to do the integration with the ServiceFacade.

[asutcl]

I have pulled from master and the changes are in branch Issue72. There are few errors that come up but I think its in code that isn't used any more. The SiteController used to be able to change the default value for a ScoredAttribute but I have removed this feature because they are now singletons and they can be accessed by multiple instances. I don't want to merge into master until these errors are resolved.

@prmr If Issue0075-mpr is in a good state I could merge it into Issue72 and go over the merge conflicts if any so that when I merge back Issue72 into master and Issue0075-mpr gets merged into master there should be no conflicts. Or is there a better way to proceed?

@MariamN or @nishanth1991 is the code setting a default value in the ScoredAttribute still necessary since we are now focusing on the importance of an attribute as opposed to its target value? (line 379 in SiteController master branch and a few instances in tests)

[prmr]

Probably best to leave the two streams until we have a chance to discuss in person tomorrow. The changes in #75 are mostly getting rid of stuff, so with a clean facade to AttributeExtractor it should be relatively easy to rewire things.

[MariamN]

No it is not necessary any more. We are not taking any specific values for the features.

[prmr]

Just merged into master. I suggest you keep this branch around for the rest of the cleanup and simply merge master into it after @asutcl's integration.

[ceipher]

Nevermind, I think I know why this extra GET request happens

[forgues]

Is anyone working on fixing the attribute selection bug? Right now I've been debugging with the console, but I won't integrate the refactored ProductRanker until I can test the integration using the website.

[ceipher]

I made a temporary fix in my branch, do you want me to merge to master?

[ceipher]

@forgues ok the fix is in the master now. Note there's no submit button. Simple click on attribute will launch the sorting

[forgues]

Thanks for the fix! I'm debugging the refactored product ranker, and it seems like the ranked product list is displayed backwards to the user. The first product in productsToDisplay (in SiteController) is actually displayed last when using the website's UI. Does anyone know where the list might be getting reversed before being displayed? I can't find it in the code.

[asutcl]

I noticed this too. After selecting a few attributes, if you deselect all of them the list will appear in reverse alphabetical order. It may be due to the fact that the item divs are being created sequentially from first to last and thus it acts like a stack and the last added item is on top, but I haven't looked at the implementation so this is just a guess. It may also be browser dependent, I haven't done any web in a while.

[forgues]

I found the line: in src/main/webapp/js/features_form.js, line 117: I replaced prependTowith appendTo

And now the list is in the correct order. Is there a reason why the list was being displayed from last product to first? Or can I safely make this change?

[forgues]

I merged the refactored product ranker into the master branch.

[prmr]

Is there any movement here? If this issue is stalled @enewe101 this is definitely a place in the critical path. Please check with @priyasidhaye.

[enewe101]

Ok will do! On Apr 2, 2014 4:38 PM, "prmr" notifications@github.com wrote:

Is there any movement here? If this issue is stalled @enewe101https://github.com/enewe101this is definitely a place in the critical path. Please check with @priyasidhaye https://github.com/priyasidhaye.

Reply to this email directly or view it on GitHubhttps://github.com/prmr/Creco/issues/75#issuecomment-39380339 .

[priyasidhaye]

I have been working on it, but I ran into some errors while separating the code. Will fix it and update

[priyasidhaye]

Completed, have pushed it to the branch RefactoredSiteController.

[priyasidhaye]

@ceipher turns out we did the same changes in parallel in cleaning up the site controller, I think your latest commit completes this issue too.

[ceipher]

oh... ok....

[enewe101]

Hey Priya, looks like this issue is complete -- is that so? You can go ahead and close it. If you think there is anything related that should get addressed for release 1.0 feel free to open a new issue tied to that release.

[priyasidhaye]

Yes, I think it is complete for now. Closing.