They asked to check the code, what should I look at?

Question

Laravel

They asked me to check if the code is good or not. I don't really understand how to do it.
Are there any criteria for checking the code?
Where can you see them?
Framework Laravel5

Answer 1

Looking why)). The criteria I use when doing a Code Review are:
* Security:
- Each simple type method argument must be type checked if it's being proxyed and bounded if it's processed. Slightly that not so - the exception is thrown. If a method with a bunch of arguments consists of 80% verification from arguments, this is quite normal))
- No trigger_error, only exceptions.
- Exceptions MUST be human-readable, all sorts of "Something went wrong" can be given to the user, but an exception with a stack trace and a human-readable description of what went wrong there should get into the log.
- Each argument (object) of the method must be type-hinted to this class or interface.
- For eval, as a rule, I send to ** th.
- @ is only allowed in hopeless situations, like checking json_last_error.
- Before working with the database - a mandatory data check.
- No == and !=. With swtich - the only exception, according to the situation.
- If the method returns not only bool, but something else - a hard check with ===, or !== is required.
- No conditions with assignments inside. while($row = ...) - also goes through the woods.
- Magic getters/setters are allowed only in hopeless situations, otherwise they are prohibited.
- Concatenations in sql - only in hopeless situations.
- Parameters in sql - ONLY through placeholders.
- No global variables.
- Dates as a string are allowed only in templates and in the database, in php code it is immediately converted to \DateTimeImmutable (in hopeless situations, \DateTime is allowed)
- Of course, it depends on the project, but as usual there should be only two entry points: index.php for web and console (or something else to be called) - for the console.
* Codestyle PSR-2 + PSR-5 at least + a bunch of more stringent requirements (for starters, everything that is marked as SHOULD in PSR becomes MUST)
- In PhpStorm, not a single line should be highlighted (the exception is typo errors, for example, a dictionary does not know any of the abbreviations accepted in your project). It is allowed to use /** @noinspection *** */ for desperate situations.
- If someone says that he writes in another editor and it is not highlighted, THIS male sexual ** is put on these excuses and sent for revision)).
* Code organization:
- No global functions.
- Classes without a namespace are allowed only in exceptionally hopeless situations.
* Testability (in the sense of ease of testing) of the code should be high.
- Code coverage is mandatory for all possible use cases of each public method with dependency mocks.
* Principles of MVC:
- No handling of user input in models, from the word at all.
- No *** queries to the database from templates.
- No layout/js/css/sql-in in controllers.
- In models, NO MAGIC, only private properties + getters with setters.
- In models, it is allowed to use the save method (if there is one, of course) only in exceptional situations. In all others - either insert or update.
* Principles of SOLD:
- No divine objects that can do everything.
- If the internal method is private, no public.
- Static methods are only allowed if there is no way out.
* The principle of DRY is allowed to be violated in cases of:
- Explicit separation of duties
- In tests (each test should be as independent as possible)
* Working with the database:
- The query in the loop must be REALLY justified.
- For ORDER BY RAND () - I send to ***.
- Search not by keys (of course, if the table is NOT 5 rows) is prohibited.
- Search without LIMIT (again, if the table is NOT 5 rows long) is prohibited.
- SELECT * - disabled.
- Denormalization of the database must be justified.
- MyISAM is not used (so)))
- Multiple operations are required in a transaction, with a rollback if something went wrong.
- The database should not contain business logic, only data in a holistic form.
- There should be no inappropriate twitching of the database where you can do without it.
* The cache must be cleared according to two conditions (not one of them, but two):
- Time.
- Rotten business logic.
Allowed for only time in desperate situations, but then the time is a short period.
- When calculating cache keys, a variable from the application configuration must be used (in case of updates, the cache is reset by the code, not by the cache server flash). In the case of using multiple servers, this is a very convenient and flexible tool for diploma.
* About people:
- "I'm used to writing like this and will continue" - no question, you will pass the review only when you change your mind.
- "I write in vim and it's so convenient for me" - great, I also write code with the console in it)) but there are requirements for the code, if you can't do it, you won't pass the review.
- "I copied this terrible method and changed 2 lines" - this is certainly wonderful, but according to the blame, you are the author of this whole method, so let's not shit, okay?
- "It works!" - this phrase is translated something like this: "yes, I understand that I am writing complete crap, but I can’t write normally because my hands are made of jo", did I understand you correctly?))
- "Everything works for me!" - happy for you, but what about the production?
- "Everything is simple there" - do not use the word "simple", from the word "completely". Here's a piece of code for you (the first one with complex business logic), where is the error (it doesn't matter if it exists or not)? You've been watching it for 2 minutes already, what's the problem, everything is "simple" in the same place))
* Anything:
ActiveRecord (I'm telling you as a Yii fan in the past) - complete shit, take it as the original one. In fact, you have models with a connection to the database walking uncontrollably around the project. More than once I came across the fact that in the same templates they call save,
The fact that Laravel is used is sad ((. To fulfill the requirements above, you have to "fight" with the framework.
This is far from a complete list of requirements, it depends a lot on the project as a whole and on the principles laid down in it. For large mregzh requests 200 code comments is ok..
UPD
Formalized these criteria by reference: https://github.com/index0h/php-conventions

Answer 2

All the commentators have made the same management mistakes because, with all due respect, they most likely do not pay for these mistakes (in strategizing) out of their own pocket.
I answer your question with fingers:
1) By structure - when checking the quality of the code / solution / task / product / server settings and so on, you need to go through the list (checklist) of quality control criteria - they usually look like lists of certain parameters that can be measured by a third the person or the system itself - the format of the parameter being checked directly matches / does not match. How many percent the checklist is passed - how many percent the result is "quality"
2) Why did the guys make a mistake - because they began to give specific lists. The fact is that each project / situation / team / set of competencies has its own sets of such checklists for different situations. In large teams, there is a main checklist that regulates CodeReview - and as a rule, the team leader is responsible for it - he updates it, develops it, justifies the rules introduced and makes sure that BEFORE starting development, all developers are IN ADVANCE FAMILIARIZED with this quality control procedure, and all because what:
3) There are a huge number of style guides and criteria in principle - and how convenient it is for everyone in one part of the world / company to do one thing - does not regulate even once that it is exactly the same for another person in another situation to apply these rules to their context. In the form of open style guides, they exist for the accumulation of practices and skills, primarily for their own development (the process of formulating puts things in order in the head) and also make it possible to “string on them specifically” the point answers of a huge community of people, and get those very different views on situations , and if possible again bring to a common denominator. But these are all the little things in life, and in your case, you will make a serious mistake if you undertake (take responsibility) right now to check someone else's code for evaluation, because:
4) You are clearly being used as an external expert who can be referred to, from whom you can allegedly get arguments to put pressure on your position when solving some situation that has arisen in the client-developer relationship on the project where you are invited for expertise.
If you, without warning, that "code quality" begins with a declaration of this quality (if it is about checking this internal quality within the framework of cooperation, and not the tasks themselves that are set for the system being created - features) - any of your assessment will be is unreliable for the context of its application (you write about lines or something else - and a person will either be charged money / or underpaid for work / or encapsulate in an agreement post factum for the same money work on compliance with certain styles - this is all the work that should be paid). Therefore, here is a fork of your actions:
1) If young colleagues simply ask you for mentoring, give them a link to Google and the key phrase php style guide github
2) If you are asked (or you yourself are such a customer who is looking for something to cling to in the code to push his position) - there are no criteria for the quality of the code BEFORE the start of work signed on paper / sent by mail - no criteria can be applied to the current relationship - only to the next iteration for the next money.
3) If you are still a developer and you were asked to evaluate the code - bring this situation to the stage of correctly closing the current stage of work - but then suggest the introduction of a style guide if it requires it. I guess not really. By giving now an answer to the question in the form of an assessment of the quality of the code, you will do only one thing - absolutely unreasonably give an argument in a clearly skewed dispute, and simply take on another bag of dirt that you will work out for some more time.
Think well on this subject - you will have to choose your side.

Answer 3

Laravel uses PSR-2 !

Answer 4

First understand why you need to "check" the code, then you will understand "what" to check. Nevertheless, I will give a summary of the criteria adopted by us
The list is far from complete. It all depends on the project, the task being performed, the platform and the competence of the team. They often ask to check the code before accepting it for maintenance. Declare your own set of coding rules and build on it.
PHP, unfortunately (or perhaps fortunately), is not a strict language...

Answer 5

On the code!)
Sory could not resist!
Well, I usually look at:
1. the purity of functions
2. the overloaded functions
3. usually the one who does the review, you know the project, look at duplication, maybe this is already in the project, but the programmer did not see this and wrote his own solution.
4. on codestyle, it should be the same for everyone. names of variables, functions, classes... they must be meaningful.
5. prints that they forgot to cut out during debugging, but usually there are tools for this.
6. a lot depends on the project, for example, if you have 5k per minute, you cannot do update view + 1, it will slow you down a lot. unless it blows up the whole system.
Well, probably a lot more, I can’t remember everything right away, but usually it’s visible in the code.

Answer 6

If you don't know, then nothing will help.
You need at least a little to disassemble the govnokod. And at different application levels. And first you need to write it yourself, and then edit it many many times.
At the application configuration level, things are not so simple. This is where a lot of experience is needed.
At the development level - you need to participate in several projects with several developers and a few departed. Then the sharp corners will immediately begin to emerge.
Well, read the classics, for example, on the subject of "code smell", support for someone else's code and good code.
If the code is readable, then it is easy to fix it even if there are gross errors. It is code editing that is the basis of development. Lack of knowledge and experience leads to bad code. Lack of coding discipline leads to useless code. Bad code can be improved. Useless - throw out (rewrite).
The only remark is that if you encounter terrible code, then you need to accept it as it is. Emotions won't help. No strength - do not take it. There are forces - step by step for now ...)