Refactor DetailedClassInformation #87
Conversation
Codecov Report
@@ Coverage Diff @@
## master #87 +/- ##
==========================================
+ Coverage 83.85% 85.13% +1.27%
==========================================
Files 10 10
Lines 254 269 +15
Branches 38 42 +4
==========================================
+ Hits 213 229 +16
+ Misses 41 40 -1
Continue to review full report at Codecov.
|
| // if there is no end string, set idxEnd to where it would be | ||
| if (idxEnd == -1) { | ||
| idxEnd = requirementsInfo.length; | ||
| } |
There was a problem hiding this comment.
@keithradford this is a simple solution to the end index, although after looking through a lot of test pages I think this is the best way to handle it for now. It seems to me that occasionally the end string is not included, and I have not been able to find any pages that include any alternative to: 'This course contains prerequisites please see the UVic Calendar for more information'
J-Raymer
changed the title
| if (idx === -1) { | ||
| return data; | ||
| // regex statement to grab requirements, ignores "restrictions:" title | ||
| const regex = new RegExp('^(?!restrictions).*:$', 'i'); |
There was a problem hiding this comment.
is there a reason for using the regex constructor as appose to using a literal? afaik declaring literals are better in terms of performance which would be beneficial for a data parsing function such as this. typically a regex constructor would be used when the expression is dynamic which I don't believe this one is.
| const regex = new RegExp('^(?!restrictions).*:$', 'i'); | ||
|
|
||
| // list of requirement indeces for parsing | ||
| let requirementsIdxList = [] as number[]; |
There was a problem hiding this comment.
i think it's better TS practice to declare arrays like: let requirementsIdxList: number[] = []; instead of type casting
| requirementsIdxList.forEach((el) => requirementsList.push(requirementsInfo[el])); | ||
|
|
||
| // initialize requirements object | ||
| let requirements = {} as Requirements; |
There was a problem hiding this comment.
same here, could be declared like let requirements: Requirements = {};
There was a problem hiding this comment.
LGTM, but I'd go through and remove any typecasting if you can. I think it's just better practice to declare types as appose to typecasting. It's like int i = 0; vs. char c = 0; int i = (int)c; in C.
Also, we might have a new branch dev we're merging to, I'm not too sure what's going on there waiting to hear back on that, so if you can hold off on merging until then 👍
No description provided.