Refactor: Tim D. Code Review - Cleanup

[thebearingedge]

Hi Steve, here's a code review as we discussed via Slack. I'll do a separate UI review. The cleanup should come first as it's much easier to make behavioral and visual changes to the code when the code is clean. In some cases, I point to specific lines of code, but you should be looking out for these issues throughout the entire code base.

As always, make sure you test your app after each change to confirm that it still works as expected. Also commit after each change.

✅ Task List

Check list items that need to be done in order to complete the refactor.

• [x] First things first, all file and directory names should be lower-kebab-case. I looked in server/lib/ and there is a seemingly random mix of casing conventions like camelCase and whatever nodeMailer-transporter.js is. nodemailer is one word, all lowercase. Every file and folder in your entire project should be lower-kebab-case.
• [x] Next, sort your require() and import statements. Built-in Node.js modules and npm packages should come first, followed by your own modules. This should be applied to every file in your project. An example of disorganized imports is here.
• [x] Your codebase should have absolutely zero chunks of commented out sections. I found a huge one pretty quickly here.
• [x] You have a lot of repetitive error handling code in your server/index.js. Review the express-error-handling exercise and move your errorMiddleware registration to just before your app.listen().
• [x] Your SQL code should be indented for readability.
• [x] You have an app.jsx that simply renders home.jsx. Cut out the middleman. Just make Home your App (and rename/remove files accordingly).
• [x] I found a few empty files in your client/ directory. Avoid ever merging anything into your master branch that isn't done yet.
• [x] You are using a seemingly random mix of property initializers with arrow functions and bound prototype methods in your classes. You have two options:
  • Stick with prototype methods only (no arrow methods as they are not officially part of standard JavaScript yet). Only bind methods that break if you don't bind them.
  • Use the arrow methods but add a babel transform to make sure that they get converted into standard JavaScript
• [x] Use ES6 classes instead of constructor functions.
• [x] Your CSS class names don't follow a casing convention. Use kebab-case for all of them and not camelCase.
• [x] Don't style CSS classes from libraries. Instead create an additional class and use it alongside the library class.

[Smith-Steve]

@thebearingedge

Tim, I have double checked this and have gotten everything. A question for you. I'm going to Slack this to you as well.

1. On check mark 8, you said "You seemingly use a random mix of property initilizers ....". I was using the arrow methods that I did not intend on using outside of the component in question. Beyond that, it is true that there was no rhyme or reason to why I was using bound prototype methods over arrow functions. What is this not reason enough?

[thebearingedge]

@Smith-Steve Hi Steve! The methods of your components are generally only going to be calling each other or called by the React framework. The main reason to choose an "arrow method" is to avoid having to bind it in your constructor. The JavaScript engine interprets them as follows.

This is what you write:

class Example {
  constructor() {

  }
  aMethod = () => {
  }
}

And this is how the JavaScript engine interprets it:

class Example {
  constructor() {
    this.aMethod = () => {
    }
  }
}

It's a new language feature that is very near being official, but it's not quite there yet. It's at stage 3 of the proposal process.

https://github.com/tc39/proposal-class-fields