I'm reviewing code style and documentation. The group doesn't appear to be using Git for issue tracking and has few PRs so it's difficult to give feedback on those. I'm reviewing these partly because I have experience with Typescript and web frameworks (we are using React and Redux for our project), but not with AngularJS so I cannot make too many comments on high-level architecture decisions.

Code

trackforever/webapp/src/app/import/import-page/import-page.component.ts: options should be an enum

trackforever/webapp/src/app/import/models/trackforever/trackforever-issue.ts: timeCreated, timeUpdated, timeClosed should be options i.e.

timeCreated?: Number;
timeUpdated?: Number;
timeClosed?: Number;

There are various places where these are set to -1, which is being used as a defacto null value.

Per the official TS do's and don'ts, Number, String, Boolean, Object should not be used. Instead, use number, string, boolean, object. These appear in many files throughout the codebase.

trackforever/webapp/src/app/import/import-github.service.ts trackforever/webapp/src/app/import/import-googlecode.service.ts trackforever/webapp/src/app/import/import-redmine.service.ts:

There are three uses of the following while-loop style:

let i = 1;
while (++i <= lastIndex) {
...

Firstly, the while loop conditional should not mutate state. Second, this should be replaced with a for loop. I don't understand AngularJS or RxJS well, but it may be possible to replace these loops with mergeMaps.

trackforever/webapp/src/app/project-page/project-page.component.ts: On line 49,

.filter(i => {
  let matches = true;
  this.labelFilters.forEach(l => {
    if (!i.labels.includes(l)) {
      matches = false;
      return;
    }
  });
  return matches;
});

should be replaced with

.filter(i => {
  return Array.from(this.labelFilters)
              .every(l => i.labels.includes(l));
});

[Disclaimer: I haven't tested my replacement, so I might have messed it up.]

Linting looks good and so do most of the types!

High-Level Architecture and Documentation

You should clearly document how your components connect to each other and what purposes they serve. Consider adding a docstring to the top of each of the component TS files and adding a short README explaining the architecture.

Consider moving import-*.service.* files into separate directories.

I think it would be a good idea to clean up the organization of trackforever/webapp/src/app/. It's good for the most part, but the files at that level should be moved into folders.

Function and class names are concise and descriptive.

cse403trackforever / trackforever

Code Review 1 #6

Code

High-Level Architecture and Documentation