Table of Contents
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
Preface
As required by the Software Project course, our team peer reviewed another team's project. This project was STELLA App and the review was performed for version 2.0 of the project.
Peer Review
Installation
The frontend application comes with a detailed readme file that describes how to run the application locally or build it for production. According to the readme the npm start command should be used to run it locally, but this resulted in a react-scripts is not recognized as an internal or external command error. This error was resolved by using the npm install command before running the npm start command. It is possible that this error does not happen for everyone.
The back end application does not have a readme file (at the time of writing this review), but it would be a nice addition. It should describe, similarly to the frontend readme, how to run the application. It is important to note that this is not a huge problem though as it is a spring boot application, which is quite common so it is not hard to find out how to run one.
Additionally it might be a good idea to create a main readme file for the entire project, which would point to the relevant resources in the project without having to search for them separately in the subfolders and the wiki.
Code review
The project is divided into the frontend and the backend part, which makes a lot of sense, as it is a recommended approach these days and in this case the frontend and backend also use different programming languages, which makes it an even better solution.
Frontend
The code of the frontend react application looks good. It has been divided into smaller components, which is one of the main benefits of choosing a framework like React in the first place, and the code is well structured and organized, thus making it easy to read for others as well. There are not any obvious antipatterns, at least in the eyes of less experienced developers. In general everything is fine and it is only possible to nitpick about some smaller details.
For example, it is possible to make the code shorter by removing some redundant statements. In the ClientInfo.js there is a following condition on the line 57: !nameCheck(firstName)||!nameCheck(lastName)||!emailCheck(email). This always evaluates to true because this is placed inside of an else branch of an if statement with a condition: nameCheck(firstName) && nameCheck(lastName) && emailCheck(email). If this statement is false then it automatically means that the statement on line 57 must be true.
The texts in the code are hardcoded and there is at least one that is in mixed English and Estonian („firstname ja lastname“), which is a little confusing. Maybe it would be a better idea to use translation files instead of hardcoded strings, even just for the order tracking part of the application that is intended for customers, as the main website of Stella supports Estonian, English, Swedish and Finnish languages. This would make it very easy to add support for all the languages later on.
Backend
For the backend application, the biggest problem is that there are a lot of hardcoded values like URIs, tokens, header values, email addresses etc. Right now all of the values are set for test environments, but at some point the application will be deployed to a production server and all these hardcoded values will have to be changed then. Since they are scattered all around the codebase, it’s likely that some important value might be missed.
Also, having this kind of data in a resource file ensures that once the client needs to change some of these values, they can just modify the resource file and reboot the application instead of having to modify source code.
Another small issue for backend is that some of the methods are a bit too long and some parts of code are repeating. In this case it would be beneficial to extract the repeating code. An example of this is in the DatabaseService.java and ProductStatusService.java files, where methods deleteDiscount(), generateDiscountCode(), returnStatus() and updateClientsProductStatus() all create a RestTemplate and add an URI and headers to it. This could be done in a separate method with all relevant data passed as parameters, which would also simplify unit testing.
Another comment (not an issue) is that many of the Model classes have unused getter and setter methods. Since this is a WIP project, it’s likely that those methods will find use at a later date, but if not, there’s no point creating a bunch of unused methods. Creating getters and especially setters is something that’s best done on a need-to basis, so that you won’t accidentally create a setter for something that should be a read-only value (this is assuming those exist, as we are not familiar with the business logic, only guesses can be made here).
On positive sides, the code is mostly very well commented and where it isn’t, self-documenting code practises are used.
Acceptance tests
The functionality was checked according to the release notes in the wiki. Only the functionalities that were marked as delivered or mentioned in the project plan scopes for iterations 2 and 3 were checked. The bugs mentioned in the release notes were ignored because it would be pointless to point out something that the development team is already aware of and also things that belong under the iteration 4 tasks because the deadline has not passed yet. In general the functionalities mentioned in the release notes are implemented and they seem to be working as intended (minus editing the existing table rows part, which according to the project plan seems to be a task for iteration 4 because the creation of update endpoints is mentioned there).
- The clients page of the admin panel works as described in the user story 4. All the necessary fields are present and it is possible to delete existing clients.
- Adding new clients, functionality that is mentioned in the user story 5, is straightforward and works as intended. The input values are checked and when everything is correct a new client is added to the database and the user (in this case admin) gets a notification.
- There is a registered products table in the admin panel, as mentioned in user story 6, which contains all the necessary fields.
- User story 7, which is about adding new products to the registered products table, works as intended, and when the product is added a notification appears in the browser and also the new product appears in the list together with all the previously existing products.
- User story 8 is implemented and it is possible to send emails to the clients. The email that was sent as part of this test was received immediately.
- Discount codes, that are mentioned in user story 9, are generated for the clients according to the number of bought bags, but there is a small issue with this functionality that is described at the end of this document.
It is really difficult to find anything wrong with the functionalities except some little details. For example, on the product tracking page, where the client can track the status of the ordered product by entering its ID, there is an option to select a display language, but when clicked on the English, Deutsch or Suomi option the application just navigates the user away from the product tracking page, which is not predictable behaviour. If it is not possible to switch languages then maybe it would be a good idea to not present the user with such an option, which would make things less confusing.
There seems to be a case of missing functionality when it comes to the user story 9. In the clients page we can see that discounts are created for clients who have ordered at least 3 bags and for everyone else the discount code is marked as -, including for clients who have bought 2 bags. However when we read the user story 9 and its details, it is said that clients who have bought 2 bags will get -5% discount codes and clients who have bought at least 3 bags will get -10% discount codes. When we check the backend Discount.java class, only 10% discounts are mentioned there and there is nothing about 5% discounts. This raises the question, where do the -5% discounts go or if they exist at all.