Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
EntityRouteEnhancer::enhance() has a bit set of ifs/elseifs as it tries to figure out whether the incoming data has one of _entity_form / _entity_list / _entity_view set.
This would be a lot easier to read as a switch.
The switch (TRUE) construction can be used here:
switch (TRUE) {
case !empty($defaults['_entity_form']):
// etc
}
Beta phase evaluation
Issue category | Nothing is broken, so its a task |
---|---|
Issue priority | Normal, because the amount of impact is not that high, but ensuring that quite an important class is actually readable is quite a huge win. |
Disruption | No disruption, for non given input the output will change. |
Comment | File | Size | Author |
---|---|---|---|
#17 | change-2218145-17.patch | 6.98 KB | mglaman |
#16 | change-2218145-16.patch | 6.97 KB | mglaman |
#14 | change-2218145-14.patch | 6.79 KB | mglaman |
#10 | routeenhance-refactor-2218145-10.patch | 6.19 KB | fran seva |
#7 | routeenhance-refactor-2218145-7.patch | 6.18 KB | fran seva |
Comments
Comment #1
Alumei CreditAttribution: Alumei commentedSomething like this?
Comment #2
GrimreaperHello,
I just review the code. It looks good to me.
Is there an impact on the interface to test ?
Comment #3
tim.plunkettIn what way is this easier to read or comprehend? I don't think I've ever seen switch(TRUE) used like that in any language.
Comment #4
joachim CreditAttribution: joachim commentedI've seen switch(TRUE) used quite a few times. I find it's easier to read, though it is a bit surprising at first.
Comment #5
msonnabaum CreditAttribution: msonnabaum commentedThis is trying to solve the wrong problem. If you want it to be more readable, just decompose the method into smaller chunks, so that all you're left with is the logic, which then becomes much easier to read.
The switch statement does not improve readability.
Comment #6
fran seva CreditAttribution: fran seva commented@msonnabaum I think the same but also think the use of switch can also improve the readability
What do you think about this chunks of code? I've separated the last ifelse statement in a new function to reduce the nested "if's".
I'm not sure if this is a correct practice in Drupal core development.
Is this the way you are thinking?
Comment #7
fran seva CreditAttribution: fran seva commentedCode refactor propousal.
phpcs shows:phpcs: Protected method name "EntityRouteEnhancer::enhance_route_entity_view" is not in lowerCamel format, it must not contain underscores (at line 95)
But I don't know if this is the right coding standard.I've seen another issue where it was said to not use lowerCamel #1533250
Comment #8
cs_shadow CreditAttribution: cs_shadow commentedComment #9
fran seva CreditAttribution: fran seva commentedIn [1] @TravisCarden confirm that the phpcs Drupal standard is correct. I will update the code and create a new patch ASAP ...this night :P
[1] https://drupal.org/comment/8664591#comment-8664591
Comment #10
fran seva CreditAttribution: fran seva commentedPatch with my proposal without Code Sniffer errors.
Comment #11
dawehnerWhile I am not convinced that the switch is better than than just a bunch of if/elseif/ statements moving the big hunk into a separate method is certainly a win.
Nope, {@inheritdoc} does not solve all documentation for you
Comment #12
tim.plunkettLet's keep the improvements and ditch the wonky switch.
Comment #13
cmanalansan CreditAttribution: cmanalansan commentedTriaged as Novice:
Latest patch here:
https://www.drupal.org/node/2218145#comment-8673313
Comment #14
mglamanHere is an updated patch which moves each check's modifications to the defaults into its own protected method, per #12
Comment #15
dawehnerit should be enhance or ehnance ...
The doc standard is using full qualified class names (FQCN)
This method throws exceptions so we should better document with @throws
Comment #16
mglamanHere is updated patch per review in #15
Comment #17
mglamanApparently the word enhance is impossible for me to type properly. Interdiff from #16 still relevant, this just fixes typos.
Comment #19
dawehnerThis looks great for me now!
Comment #20
alexpottThis needs a beta evaluation - I agree that the proposed change makes the class much more readable and therefore less fragile when making change. So I think this is permittable under committer discretion of reducing fragility.
Comment #21
dawehnerSure
Comment #22
alexpottCommitted 2f853b1 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.