clean-code-php
clean-code-php copied to clipboard
Discussion about "Don't add unneeded context"
Hi, I would like to bring up a discussion about "Don't add unneeded context". I have few considerations why I think this very example is not good and the context is not unneeded. On the contrary - on ActiveRecord classes it is improves greatly searchability, traceability and prevents hard to detect bugs:
- I find extremely useful the properties to be named the same way for the sake of traceability from the front end (field names in forms) all the way back to the database column names
- because of the above not having a prefix and having just column names like "color", or "name" (think of user_name, role_name, bank_name) and having of join of three tables like "users", "roles", "bank" and forgetting to set aliases will produce wrong results. The last selected table will overwrite the names of the rest. Consider having not just three tables but a join of 10 or even more... with tens of columns each... It is a lot of typing to alias many things. And if there is:
SELECT users.*, roles.* ...
then things go really bad. When there are hundreds of returned columns overwritten values go unnoticed. - In regards to the front end - in a form where in one go (read one transaction) we need to create a User and a Role we will need to add prefixes to avoid collision of the "names" (role_name vs user_name).
- Searchability - if I need to find out where the user name is used I can search for "user_name" while if I dont have the prefix I will need to search for "->name" which will return also Roles objects, Banks objects... The object may or may not be named "$user" in order to make a search "user->name". Also this way it can be searched in SQL queries, frontend...
I would love to hear your opinion on that. Thank you
It is worth referring to the primary source. There is a Don`t Add Gratuitous Context section on naming classes in the Clean Code book, but these rules also apply to naming class properties (I was not able to immediately find the text on which the Don't add unneeded context section is based).
In an imaginary application called “Gas Station Deluxe,” it is a bad idea to prefix every class with
GSD
. Frankly, you are working against your tools. You typeG
and press the completion key and are rewarded with a mile-long list of every class in the system. Is that wise? Why make it hard for the IDE to help you? Likewise, say you invented aMailingAddress
class inGSD
’s accounting module, and you named itGSDAccountAddress
. Later, you need a mailing address for your customer contact application. Do you useGSDAccountAddress
? Does it sound like the right name? Ten of 17 characters are redundant or irrelevant. Shorter names are generally better than longer ones, so long as they are clear. Add no more context to a name than is necessary. The namesaccountAddress
andcustomerAddress
are fine names for instances of the classAddress
but could be poor names for classes.Address
is a fine name for a class. If I need to differentiate between MAC addresses, port addresses, and Web addresses, I might considerPostalAddress
,MAC
, andURI
. The resulting names are more precise, which is the point of all naming.
I will add my opinion.
You are using a outdated and not popular style of table column naming. This style has both advantages and disadvantages. I don't want to condemn your choice in any way.
I want to draw your attention to the fact that here we are talking about the PHP code and the entities of the domain layer. Column naming refers to the persistence level. The correct coding style is to focus on the domain level and all other layers are superimposed on top of it. The persistence layer should not dictate what the domain entity should be.
You may be confused by the ActiveRecord design pattern, which combines the domain and persistence layer. With Doctrine, you won't have such problems.
Hi Peter,
I presume by mentioning Doctrine you mean to use constructed queries with the Query Builder? I never used it as the projects I work have multiple queries with tens of joins, subqueries & unions and hundreds of columns involved (no, Im not using .* but this will work too without any collisions) and it will be impractical to use a substitute language/framework for plain SQL. In this case I consider having a prefix to the column names mandatory as then tens of columns will need to be aliased and is extremely easy to miss and overwrite. And the worst is that such errors may go unnoticed. I have seen people developing without prefixes in this scenario and critical bugs get introduced immediately (and prefixes had to be added).
Indeed the layers can/should add a prefix as they need to (for example the front end as per my example - user_name & role_name, this can be easily automated with overloading) but I think it is easier to search and trace the path of the data instead of tracing "user_name" and sometimes (in a different layer) just "name". But of course this is down to own preference and may not be that critical depending on how well the developer knows the used framework & specific project he is working on.
Also Im probably biased as the projects I worked have tighter relation between the domain and persistence layer as due to the complexity of the applications it would be cost prohibitive to ever change the persistence layer (the queries are DB specific, hundreds of them, there are functions, triggers...) and we have opted for tighter integration for the sake of gaining extra visibility/traceability in the code (the projects are very large - one of the apps is >200 000 loc, excluding framework & libraries).
Another thing I just thought of is that in this kind of projects there are tens of similarly named classes (with almost the same properties). If no prefixes were used on their properties it has happened the developer to create an instance and use it meaning to use an instance of a different class (especially valid when an IDE with autocomplete is used... people click fast and import fast). When prefixes are employed he/she would immediately get an error for non-existing property and discover his/hers error. And it is very easy when debugging to just ping someone over the chat the name of the (prefixed) column instead of sending the whole class name & property name (the fully namespaced class names tend to get very long and easily confused as mentioned).
Thanks
P.S. I fully understand the advantages & flexibility of the separation of domain & persistence layer. And my point is not that the persistence layer has to "mandate" how the properties are named but for human readability & bug avoidance I feel it is better to have more explicit names, not because they are "needed" by the persistence layer to avoid collisions. This can be always solved with the framework.
I propose to draw your attention to the Doctrine, because it is the Date Mapper. It allows you to name the columns in the database in your own way, and the fields entityes in your own way. That is, the user_name
column in the user
table can be mapped to User::$name
. Also, Doctrine automatically uses aliases and you will never have collisions with the names of fields/columns if you do not select one and the same fields explicitly.
If no prefixes were used on their properties it has happened the developer to create an instance and use it meaning to use an instance of a different class (especially valid when an IDE with autocomplete is used... people click fast and import fast).
This is exactly what the book says. Your example is highly exaggerated, since large projects are divided into modules and IDEs are smart enough not to suggest that you use a class from another module.
And it is very easy when debugging to just ping someone over the chat the name of the (prefixed) column instead of sending the whole class name & property name (the fully namespaced class names tend to get very long and easily confused as mentioned).
Honestly, i do not understand how easier it is to write user_name
instead of user.name
or User::$name
in discussion.
It is not necessary to specify a namespace. If the project is divided into modules, then it is easy to understand from the discussion which module is in question, and within one module there are usually no classes with the same names.
In regards to the DataMapper and the aliasing in Doctrine this will require to use their EM query builder which is not practical for very large queries. Becomes unreadable and harder to maintain in my opinion. Of course for smaller projects this is not an issue.
Your example is highly exaggerated, since large projects are divided into modules and IDEs are smart enough not to suggest that you use a class from another module.
I have seen this happening many, many times - wrong class to be used. Of course people use different IDEs and configure them differently.
Honestly, i do not understand how easier it is to write
user_name
instead ofuser.name
orUser::$name
in discussion. It is not necessary to specify a namespace. If the project is divided into modules, then it is easy to understand from the discussion which module is in question, and within one module there are usually no classes with the same names.
I mean that it is easier to type "unique_prefix_name" (the property prefix is unique project wide) than Some\Very\Deep\And\Long\Namespace\Coming\Here\User::name (and it is less prone to be misread I think). In a conversation & docs complete namespace has to be provided as otherwise mistakes will happen. Even within a single module there could be tens of classes and some will have similar names (as they do similar things).
My overall experience shows that saving some typing to avoid the indeed very obvious repetition like in the given example ($car->carMake vs $car->make) is minor gain compared to the benefit of code tracebility (I have to go through the mapping in Doctrine for example, then in the frontend there could be another alias, to trace the path of a piece of data) and avoidance of certain class of human errors. Im not saying that the whole class name should be repeated in the property, but just have a unique (for the class) prefix of the properties for larger projects.
P.S. there are technical reasons why I dont use Doctrine - does not properly support partial rollback on nested transactions, does not support automatic transaction rollback (when the scope is left unexpectedly) and other reasons. These are mandatory requirements in our projects.