Skip to content

Instantly share code, notes, and snippets.

@liberium
Last active October 7, 2018 18:32
Show Gist options
  • Save liberium/f5d3ba31eb4c434611a96683d934f502 to your computer and use it in GitHub Desktop.
Save liberium/f5d3ba31eb4c434611a96683d934f502 to your computer and use it in GitHub Desktop.
Code review test task for Spdload

Module structure

  • Webpack path alias should be added for top-level modules: components, screens, store, etc.

    Justification:

    import { Scale } from 'components' is more succinct than import Scale from './../../components/Scale'.

  • components/ should be a Node module: contain index.js with import-export statement for each component submodule.

    Justification: this way, we would be able to import { A, B, C } from 'components' instead of writing three statements.

  • Each subfolder of components/ should be a Node module: contain index.js with import-export statement for a component.

  • For each Node module in src/, its index.js should only contain import-export statements.

    Justification: in the Node.js world, index.js of a module is dedicated to define the module's interface. It's name doesn't speak for itself, so developer opening the file doesn't know exactly what to expect to find there.

  • All the objects related to the Redux store should be consolidated in the store/ module.

  • If we did not decide to make screens the only container components, containers/ module should be introduced.

  • The networking code should be decoupled from the state management code and placed in api/ module.

  • services/ module should be eliminated in favor of two: api/ and util/localStorage. No need in the history module: it's trivial and casually used.

  • routes/ should be eliminated. PrivateRoute and PublicRoute should be contained in components/.

    Justification: PrivateRoute and PublicRoute are pure though not visual components.

  • WithRole HOC should be moved from helpers/ to components/.

    Justification: it's a HOC, not a helper function.

  • helpers/ should be renamed to util/ and contain only utility functions and classes.

Component structure

  • (minor issue) Exported pure components should be functions, not arrow functions.

    Justification: official documentation follows this approach. Functions occupy less memory than bound function objects and are being constructed faster. Function definitions are hoisted and can be exported by default with a single statement.

  • RadioQuestionsOptions is a standalone component on its own and should not be contained in AssessmentQuestions.

  • Checkbox is a standalone component on its own and should not be contained in Formik.

React naming convention violations

  • components/AssessmentQuestions should be renamed to components/AssessmentQuestionList.
  • pages/ should be renamed to screens/.

Common naming inconsistencies

  • In .env REACT_APP_API_ENDPOINT=https://blueacademy-dev.azurewebsites.net/blueacademy/v1/ Should be: API_BASE_URL=...

    Justification: The whole app is React-based, so REACT_APP_ prefix is unnecessary. The value is not REST endpoint, but a base URL.

  • stores/ should be renamed to store/

    Justification: there is a single store in a Redux-based app.

  • src/app.js should be renamed to App.js, placed at components/ and imported in src/index.js.

    Justification: App is a pure component. src/index.js is the entrypoint of the app.

Infrastructure

  • ESLint and Prettier configs should be kept in package.json.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment