Code Review - Predictably

Code formatting

  • Code is formatted in same manner through the whole source code with very few exceptions.
  • Indentations are mostly right and lines of code are not too long, with exceptions for strings.
  • The naming of files differentiates between the modules, it doesn’t follow the certain naming convention.
  • There’s a lot of commented out code which should have been removed as it makes code less readable.
  • Console.logs are left in the code where they shouldn’t be.
  • CSS doesn't follow any specific styleguide

Architecture

  • Code is divided into modules, and then into client and server. Express server is serving Angular as the View in MVC. If there's a plan for adding more clients in the future (mobile application, for example), it'd be better to have a proper client-server architecture, where the client application will be served separately from the API.
  • Relational database is used. The choice of the database is good, but the model is not optimized for the DB calls that are made through the app, slowing down the performance. Database schema should be remodeled. It’s unclear at first whether MySQL or PostgreSQL is used, as both appear in the code and configurations.
  • A lot of handling that should be done on the server is done in the client-side instead: filtering, defining SQL queries, more advanced business logic.

Following best practices

  • Angular styleguide is not followed: files contain more than 400 lines of code, functions are too long, angular components are not wrapped in IIFE, and so on.
  • Most of the time filtering, sorting and similar is done by writing custom functions that are not optimized, instead of using a library like lodash to make it more readable+faster (code run in the background when using lodash is mostly using more advanced algorithms).

Non-functional requirements

Maintainability

  • Code is following a certain coding style, in terms that it's properly indented and lines not too long, which makes reading easier.
  • ‘Core’ and ‘user’ modules are well commented and can be relatively easily followed.
  • There’s a lot of commented out excess code that should be removed.
  • Modules other than ‘core’ and ‘user’ are not having comments describing why is something done, thus getting into this code and understanding what's going on takes way more time than it should.
  • Code coverage is low, with only core and user modules being partialy tested.
  • Code can’t be easily tested due to being overly complex (and is not well covered with tests) and being badly organized, in terms that logic is on client instead on server.
  • There's no database seed/populate, to make running of the app locally easier.
  • Code is too complex, with up to 5 nested loops, making it hard to debug, and also bringing down the performances.

Reusability

  • There’s a lot of relatively similar code that couldn’t be grouped better as it is now because it’s too strictly connected to the data model. It's not DRY.

Reliability

  • Server side is handling errors properly, returning proper error codes.
  • Client side relies too much on current data structure, accessing the nested attributes in objects without previous checking if the values for keys exist, which might lead to a problem. Method get from lodash or similar should have been used instead of directly referencing an attribute, to prevent possible code breaks.

Security

  • Passwords and sensitive data are encrypted.
  • There are policies defined to handle authorization.
  • Due to SQL queries being made from client side (client is just passing a query to be executed on server), the app is prone to SQL injection. Using sequelize type to prevent the injection attack is not secure enough, making queries shouldn’t be done from client.
  • Default admin is defined in configuration file without using any environment variables, making it the same for all the environments - and easy to find out by reading source code.

Performance

  • There are many nested iterations over dataset in code, many having up to O(n^5) complexity or more, that lead to worsened performance.
  • A lot of the calls to the database consist of joining up to 4 tables, which is costy (time-wise).
  • When profiling admin pages (for example /home/commodity-price or /home/evolution page), it takes up to 25 seconds to load a page. Scripting time is around 16-17 seconds.