Codebase

[samhstn]

I have really enjoyed the discussions I’ve had with @sohil about design changes in the codebase and it has really caused me to question some of my principles and spend some time looking into design patterns.

I have found this repo to contain some good guidelines for javascript design patterns.

The following is a summary of the points I would like to raise about the codebase for further discussion:

Refactoring that has already been implemented in pr #298 (Mostly noted in #290 and #297):

• Node version upgrade
• Front and back end testing divide
• Endpoint grouping through Server plugins Reasons here
• Test runner instead of globbing pattern Reasons here
• Removing the backend test transpiling
• Starting the change over to es6 syntax in the backend We are already using es6 in the front end it would make sense to keep the code style consistent
• Server endpoints testing refactor
• Database helpers testing refactor and implementing a db setup to make the tests orthogonal

Higher level changes that I think would be good to implement in the project:

• Add a code climate gpa review so that we can get a breakdown overview of where the repeated code is
• Implement some kind of tool to find unused files/code (this might be offered by code climate)

Further refactoring that I think would be good to implement on this sprint:

• Ensure consistent levels of abstraction in the handlers (for now putting the database helpers in server/lib) (<30min)
• Remove unused code (Since we have version control we aren’t going to lose the work done if it is useful in the future, but if it’s not used lets not keep it in the codebase) (1hr) These would be react tests and files that come up from the “unused files” tool (I found server/lib/init-db.js manually)
• Instead of validating on the first few lines of each route handler, use Joi for validation (2hrs)
• Database config implemented in a single place (note made here) (<30min)

Refactoring that I think should be implemented some time in the future:

• t.plan I can’t see many use cases for t.plan, it allows you to know how many assertions you will run which most of the time is useless to know. The only use cases I can think of are something like this and the example mentioned here. It also mentions in the article that “going for simply t.end will save you the hassle of updating t.plan statements with the correct assertion count” which is my main issue with the syntax.
• api re-architecture Comments were made by Tony about conforming to rest api standards) Deciding on a consistent response object to be returned from api requests would also be good Summarised in in #283
• Repetitious and sometimes unnecessary error handling in the database functions Errors are sometimes handled on every level of abstraction instead of being passed up or delt with on one level (using the promise catch syntax will make this much easier) If there are sufficient tests and validation on the request then there shouldn’t be the need for as much error handling in the database functions making them more readable Example here
• Abstracting out some of the common parts of the database helpers instead of repeating them in each function Also we should apply the principle that functions should do one thing
• Consistent syntax throughout the project es6/es5 and consistent levels of abstraction (also perhaps function signature consistency - not sure about if this is an issue but making a note of it)
• Find a better way to group/abstract out queries than server/lib/queries.json It’s good that the queries are abstracted out, but the file is unreadable. Also if we are going to abstract out queries we should be consistent
• The console has a number of errors when you run the tests, this should be fixed I think that it’s to do with trying to send emails while the xhr object is not behaving as it should within jsdom and logging an error
• Stubbing out the sendemail module. We are sending actual emails every time we run the tests. Not only is it filling up Sohil’s inbox, it is also causing our tests to slow down since sending an actual network request by far the slowest thing going on when our test run, I have run the tests for two similar sized projects (in terms of back end api (both using hapi, postgres and redis)) and they run in just over 5 seconds on my computer, whereas for me our back end tests are taking 37s

[samhstn]

Reopened by popular demand

[iteles]

@shouston3 Can I suggest that you pull out the 'quick wins' and the more critical ones into their own issues (and link them above) so we can discuss them one at a time? :blush:

[Danwhy]

I strongly agree with everything you've raised here, and if you get the opportunity, they'd all be good issues to solve. I've added a few of my thoughts below on the codebase/project in general that I came up with while looking through it the other week.

Error Handling

We're now using Joi which makes things a lot simpler in terms of error handling, but there's still a few other things we could do.

• Set up a function to handle errors: If we have a function exclusively for handling errors we can pass it any error we receive, and it can deal with it, whether that means sending the correct status code to the front end, logging it (either just to the console, or through an error logging service, which would be a good idea to set up eventually), or both, or anything else you decide you want to do in the future.

• hapi-error: We could use hapi-error to catch the default errors thrown by hapi and deal with them as we wish (redirecting to a different endpoint/displaying a user-friendly message).

Security

One thing I noticed only because @nelsonic pointed it out on our project is that when someone fails a login attempt, you're returning a different response based on whether the user doesn't exist in the database, or has entered an incorrect password.

This is bad because it enables people to discover whether a certain email address has been used to sign up to your site. To combat this, we should be sending the same response for 'this user doesn't exist' as we do for 'this password is incorrect'. Something generic like, 'sorry, we don't recognise that username or password'.

Workflow

Not really related to the codebase, but I thought it might be useful to give some advice on git workflow.

We had some problems early on with huge, hard to read pull requests. We've managed to combat this somewhat, but there are a few things we could do to avoid this in the future.

• merging/rebasing: We should be regularly merging or rebasing with the master branch (in this case staging). This avoids merge conflicts, and stops already merged commits from showing up in the PR. This should be done at least daily, or when you know the master branch has been updated.

If you've never used the git rebase command, it might be worth looking into. It has some advantages (some disadvantages too) over git merge.

It allows you to merge two branches together without having to create a 'merge commit'. I prefer this because it means that you can rebase regularly without your commit history becoming overrun with 'merged master into branch' commits. Of course, because it does this without a commit, it means that your pre-commit hooks won't run, so it's a good idea to run your tests/linter before pushing, especially if there were any merge conflicts.

Rebase also rewrites the history of your unmerged commits, so if you've already pushed up to github, you'll have to force push (-f) to overwrite what you've already pushed. If you think there's a chance that someone else on the team has already pulled from your branch and is using that to work from, never do this, as it will create a lot of annoying problems with commit histories being mismatched. In that case, just do a normal merge.

• Another way of combatting large PRs is to open a PR as soon as you have a working feature, even if you have more work to do that depends on this feature.

In this case, just continue working from the same branch, and once the original PR is merged into master, merge/rebase and the old commits will not show up on the next PR. The only complication here is if the original PR has a lot of change requests during the review, but in that case you'll just have to do a careful merge/rebase, resolving any merge conflicts that arise, it's still preferable to a huge PR.

These are all just suggestions, and are by no means necessary to implement now, or in the future, just some advice that could hopefully make life a little easier for you 😄 Let me know if you have any questions about this or anything else