13/Apr/2018 Technical Code-Review notes

[itspoma]

• combine everything to one branch (make some build process). Actually why do you need master branch, can you do development work at cordova branch?
• use dotenv package to remove dependencies from codebase https://github.com/DmytroRoik/Project-X/blob/master/src/config.js
• add prefix is if value on variable is boolean (calendarListShow => isCalendarListShow) https://github.com/DmytroRoik/Project-X/blob/master/src/App/App.js#L17
• remove all console.log from code
• use { errors } instead of { errors: errors } for object's destructuring

[bohdanbirdie]

• Move .eslintrc from src folder to the root of the project
• Can you please try to use an airbnb instead of airbnb-base?
• Add eslint script to package.json and run it from ./node_modules

node ./node_modules/eslint/bin/eslint.js ./src

• Eslint fails. How about adding a pre-commit hook to run eslint? Or at least adding a general rule to run it before commit manually. Fix all the issues as well, and try to avoid warnings.
• Change all .jsx extension to .js since it's pretty much outdated. No .jsx extension? facebook/create-react-app#87
• Add env config to .eslintrc, because there is issue with undeclared variables (window, localStorage....)

"env": {
    "browser": true,
    "node": true,
    ....
  }

• Separate devDependencies from dependencies, so packages like eslint-plugin-flowtype and etc wouldn't be included in app build file.
• In config.js file pull app version and name from package.json instead of hardcode.
• Always keep methods ordering, render method should be last in the class, so in ./src/App/App.js method componentDidMount goes after render but should be on the top after the constructor. Also, method toggleCalendarListVisibility is not a class method but a function avoid mixing this in future and use only class method syntax aka nameOfMethod(){...}
• Using `bindActionCreators will make your life easier, here is a code example

import { myAction } from './action'
....
Class test extends Component {
 someMethod() {
  this.props.myAction()
 }
}
....

const mapDispatchToProps = dispatch => bindActionCreators({
  myAction,
}, dispatch);

export default connect(
  mapStateToProps,
  mapDispatchToProps,
)(test);

• There is a lot of places where your code is hard to read, most of them are a JSX code, so try to setup your text editor or IDE to auto-prettify code following eslint plugin rules.
• Keep similar syntax everywhere, so adding semicolumns in one place require you to add it everywhere.
• Some of the containers are pretty big, like 250+ lines of code, this is because they contain too much is business logic, which has to be moved from components to actions. You can keep only internal login in components. For example - when you have a Switch component - there is no need to have an action for it, so it can be controlled by state, but the result of toggling have to be operated in actions and saved in store.
• Local storage is case sensitive, so to avoid any issues - keep it in lower case with underscores localStorage.removeItem('Events');
• Empty tags have to be self-closed <span class="slider round"></span>
• Move API endpoint to config file https://www.googleapis.com/calendar/v3 so it will be easy to update every HTTP call
• Business logic is forbidden in reducers, so keep only store update code there

case 'ERROR_HANDLER':
    {
      navigator.notification.alert(action.payload, null, 'Room Manager', 'OK'); // this is bad
      return state;
    }

• Why do you have images folder in ./src?