Remove dead code and simplify redundant constructs#7709
Conversation
- remove the unused Doctrine FieldMapping imports left over from the DBAL 3/4 compatibility work - remove AdminContextFactory's duplicated getUser() call and the unused $request parameter of getDashboardDto() - remove the unused $entityDto parameter of CommonPreConfigurator::buildTemplatePathOption() - return expressions directly instead of assigning them to a variable used only once (SearchDto, MenuFactory, MenuItemMatcher, ComparisonType) - flatten the nested null/Stringable checks in CrudDto::getDefaultPageTitle() and use the nullsafe operator in AdminContextFactory
1265d05 to
f39621f
Compare
javiereguiluz
left a comment
javiereguiluz
left a comment
There was a problem hiding this comment.
Thanks Guillaume! Very nice changes 🙌
I reverted two minor changes, but loved the rest.
| $terms = array_map(static fn ($match) => trim($match, '" '), $matches[0]); | ||
|
|
||
| return $terms; | ||
| return array_map(static fn ($match) => trim($match, '" '), $matches[0]); |
There was a problem hiding this comment.
I reverted this change. In general, I dislike the "replace redundant local variable with a return" suggestions made by IDEs, etc.
It's true that the original $term variable is unneeded, but putting the entire expression in the return statement means that you can't easily know what are we returning here. You have to "compile the expression in your head" every time you read this code.
In some cases where the expressions are trivial, I'm OK with returning it directly without a local variable.
| 'type' => 'numeric', | ||
| 'choices' => static function (Options $options) { | ||
| $choices = match ($options['type']) { | ||
| return match ($options['type']) { |
There was a problem hiding this comment.
I reverted this change too. The returned expression is simple ... but the code is so long, that it's hard to track where the return ends. That's why I think that the code is simpler to understand if we add the local variable.
2 participants
DBAL 3/4 compatibility work
$request parameter of getDashboardDto()
CommonPreConfigurator::buildTemplatePathOption()
used only once (SearchDto, MenuFactory, MenuItemMatcher,
ComparisonType)
CrudDto::getDefaultPageTitle() and use the nullsafe operator in
AdminContextFactory