rosshamish / classtime

university schedule generation and course data API
MIT License
16 stars 2 forks source link

Refactor subjectBin object structure to meet best practices #53

Closed ahoskins closed 9 years ago

ahoskins commented 9 years ago

Right now subjectBin errors are suppressed on the filter (via a memoization wrapper), but the fact of the matter is, subjectBin is poorly designed.

Current state:

$scope.subjectBin = { 
        "Faculty of Engineering": {
            "ECE": [{<course-object>},...],
            "MEC E": [{<course-object>}]
        },
       ...
    };

Proposed change:

$scope.subjectBin = [{
        facultyName: 'Faculty of Engineering',
        subjects: [{
            subjectName: 'ECE',
            courses: [{course-object>}...]
        }, {
            subjectName: 'MEC E',
            courses: [{<course-object>},...]
        }]
    }];

The proposed change emphasizes more arrays and fixed property names. It makes iterating over the entire object less verbose, easier to read.

Note: after refactoring, there still might be a need to use the memoization function. But the scope of this problem is such beyond just the filter.

rosshamish commented 9 years ago

We should do [#55 Change courses-min to hierarchical response] at the same time as this - the proposed structures are the same.

ahoskins commented 9 years ago

56 is also kind of related. Related enough.

rosshamish commented 9 years ago

Will this affect searching/ng-filter at all? Or it will still work?

ahoskins commented 9 years ago

Yes, it will affect all logic that relies on $scope.subjectBin. So the filter, the HTML accordion directives, etc... I'm a little torn about the whole thing over the last few days because everything is is working as-is right now.

But if it's gonna get changed, I should change it before I start polishing all the features.

rosshamish commented 9 years ago

In #55 , I'll expose a new courses-min endpoint with a different name. Then, you can modify your side to accept the new response. Then, we'll change the name back to courses-min in a single commit.

ahoskins commented 9 years ago

Yes. I won't touch anything until I see that new response 😀

rosshamish commented 9 years ago

This is done - it works but is a bit slow on the backend. The speed will be fixed in #69 Fix slow courses-min hierarchy building