Video Tutorial Code Review: felixdorn / flash

I propose today to discover a new format of video, the review of code. The principle of this format is to learn new things and improve the way it develops by analyzing the code of other developers.

The goal of this exercise is not simply to criticize another's code, but rather to improve one's own way of coding by analyzing problems that one can detect on a code that one does not know. .

The subject

For this video we will analyze the PHP felixdorn / flash library. It is a library that can manage flash messages to display during certain user actions (we are here advantage because we know the operating principle of this kind of class).

Where to start ?

When you decide to evaluate someone else's code you have to go from the most general to the most precise. The project structure is first analyzed to identify certain problems. In the case of the library presented today, the structure is correct and the entry points are easily identified.

• The folder src corresponds to the root of the library's namespace
• The folder test contains the unit tests and makes sure that the library works as expected.
• The presence of a file .travis.yml indicates the use of continuous integration.
• A file examples contains examples to discover how the library works. This file is not necessarily necessary since one has the tests that can be enough to understand the scenarios of use of the library.

The composer.json file

We can now go deeper and start looking at the contents of some files and we will always start with the file composer.json.
Inside this file we look at whether the namespaces are correctly defined (in the case of the library that we analyze the declared namespace is too generic Felix instead of Felix Flash).
Then, we also check if the version of PHP necessary for the functioning of the library is defined in the part require. This argument is often forgotten and gives rise to many problems when people try to install the library on PHP versions lower than expected. Specifying a minimum version number will allow users to receive an alert when trying to install via composer.

The code

The next step is to look at the code without necessarily trying to understand the logic and trying to identify incorrect practices.

In the case of the library that we evaluate we will simply note a lack of consistency in the naming of the different methods and also a lack of comments (which will make difficult the next step where we will seek to understand the operation different classes and their roles in the bookstore). On the other hand the formatting of the code seems correct and coherent between the different classes (which allows a better legibility of the code but also to look more simply things in case of problem).

We will also note the presence of interfaces. This is usually a good sign because it indicates that some parts of the library will be replaced by different implementations.

Operation

After observing the form of the code it will be necessary to try to understand it to detect problems of logic or structure.

The absence of explanatory comments will hinder the effectiveness of this step because it is difficult to identify the role of the different classes and to understand the author's choices. Looking more closely, we also observe a structural problem with two classes that have almost identical methods and it is difficult to understand the role played by the two classes. Flash and To flash.

Also, the Flasher class seems (under certain conditions) to interact directly with the session. This choice goes against the single responsibility principle and it would have been interesting to separate the management of the session in a dedicated class with an associated interface in order to be able to separate the logic and allow the end user to replace this part more easily. -If necessary. This would also make it possible not to have to check with each method of the class Flash that the session is started successfully.

Breaking change & PR?

We note here a design error that we can not unfortunately correct without introducing major changes in the library. In this situation we will not necessarily propose changes too important to the author, because it would break the backward compatibility and require the creation of a new version. In our situation here we will be content to propose minor changes and improvements that do not break the current operation of the bookstore.
We can propose to the author to work on a restructuring of the library but it is important to create an issue before starting so as not to work for nothing. If the author does not wish to make major changes it may be wiser to create a new library on which you will have the hand.

Goal

This exercise is interesting because it allows us to notice some mistakes that we may have had to make too. When working on a bookstore or our own code it is difficult sometimes to step back and identify some problems. It is by looking at the code of others and trying to understand that we can find techniques to improve our.

If you want to propose your code for a next code review, do not hesitate to come on the chat (salon # code-review)