Post sprint feedback

[11janci]

Some final remarks after the sprint, most of it I mentioned already in the PRs.

Splitting work
I noticed that first week you focused solely on the front-end and left the back-end part for the second week. Would be interesting to hear your own feedback on that, I would generally advise starting working on both simultaneously, which would allow you to prototype the whole solution sooner and start with end-to-end tests earlier. Also, it could help highlight critical parts that still need to be done, helping you to prioritize work, so that you don't waste time on UI details that are not all that important and can be finished later.

Namings
As a programmer you'll spend most of the time reading somebody else's code. Good names can make a huge difference in time required to understand a piece of code. Names should be accurate and descriptive; it's better to have a longer name if necessary than to make it short but inaccurate. Avoid using generic names like data. Follow the naming conventions (eg. camelCase for variables, PascalCase for classes), if there isn't a strict convention, choose one as a team and stick to it.

API Error Response
Do not copy the HTTP status code in the response, or use a flag for indicating whether the operation was successful. The HTTP status code already does that, clients will use it to determine whether the action failed or succeeded.

// BAD - copies HTTP status code!
res.status(400).send({ 
  error: { 
    code: 400,
    msg: 'clientError' 
  }
});

// BAD - returning HTTP status code 400 means the action failed, no need for isSuccess!
res.status(400).send({ 
  error: { 
    isSuccess: false,
    msg: 'clientError' 
  }
});

// GOOD
res.status(400).send({ errorMsg: 'clientError' });

Use an error code when you need to give a more detailed indication to the client of what went wrong:

res.status(409).send({ 
  error: { 
    errCode: 10100,
    msg: 'Homework submission deadline expired' 
  }
});

Don't Repeat Yourself

• in https://github.com/GSG-G7/Parent-assistent/blob/master/server/controllers/routes/login.js you do

const validationErr = new Error('Please, Check the data you entered');
validationErr.statusCode = 400;
throw validationErr;

at multiple places. This should be extracted into a method in a separate module, so that it can be reused across project:

throw createValidationErr('Invalid request - username missing');

• in the client, you call the APIs directly:

axios
      .get(`/api/v1/subjects/${subjectId}/activities/${classId}`)
      .then(result => {
        const newData = result.data.allActivities;
        this.setState({ data: newData });
      });

If the API changes, you will have to update every place it is called from. To prevent that, you should extract these calls into a separate module:

api.getSubjectActivities(subjectId, classId).then(activities => { ... });

Server error handling
The error handling is inconsistent. In some controllers you throw an error, in others you directly submit an error response. Plus, most (if not all) controllers duplicate the error handling code (which touches previous point of not repeating yourself). Here's the preferred way to do it:

• have a custom error object for common errors

function BadRequestError(msg) {
  this.message = msg;
}

• then in the code throw the error of the desired type

if (!valid) throw new BadRequestError('Invalid request');

• have a catch in each controller where you just pass the error to the next function

.catch(next);

• and finally, have a middleware configured in which you handle all the errors in one unified way

switch (error.constructor) {
    case BadRequestError:
        return res.send(400).json({ errMsg: error.message });
    case NotFoundError:
        return res.send(404).json({ errMsg: error.message });
    default:
        log(error);
        return res.send(500).json({ 'Unexpected error occured' });
}

API design
I like your APIs, they follow the REST principle pretty well 👍 My only comment would be to use plurals for collections: for instance /profile/teacher/:id should be /profiles/teachers/:id. The idea is that the URL represents a path through collection(s) of resources to a specific resource. If you're interested, here is a good guideline on designing APIs: https://cloud.google.com/apis/design/

Project structure (server)
Just a few suggestions:

• I'd remove controllers/error - error handling should be done in middleware as suggested above
• controllers/middleware and controllers/utils I'd take outside and put them into the server directory
• the controllers folder should contain only files from controllers/routes
• in a larger project there should be a controller per API group, i.e. a StudentController for all APIs under the /students path. If you had one file per API you could end up with dozens if not hundreds of files...

Misc issues

• set maxAge for the cookie:

  Parent-assistent/server/controllers/routes/login.js

  Line 40 in e201e1f

  res.cookie('access', accessToken, { httpOnly: true });

• in https://github.com/GSG-G7/Parent-assistent/blob/master/client/src/components/App/index.js, this

<Route exact path="/logout" render={props => <Home {...props} />}/>

can be simplified to (try to understand why if it's not clear)

<Route exact path="/logout" render={Home}/>

---

Was nice to see you guys make progress 🙂 It was pretty visible from the code later in the sprint than at the start... Keep it up! 👍

[mohqarmout]

thank you
we enjoyed working under your instruction