Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables

File /core/modules/views/views.module

Line 125: Unused local variable $plugin
Line 196: Unused local variable $display_id
Line 1264: Unused local variable $field_name
Line 1272: Unused local variable $area_name
Line 1289: Unused local variable $field_name
Line 1297: Unused local variable $area_name

CommentFileSizeAuthor
#6 core-remove-unused-local-variables-views.module-2057157.patch2.77 KBlegolasbo

Comments

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Assigned: legolasbo » Unassigned
Status: Active » Needs review

I've located all six of these variables and have come to the conclusion that every one of them is a key in a foreach loop. I think removing these won't do the performance of their functions any good, but will reduce code understandability.

In my opinion we should leave these variables untouched.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

@legolasbo: Can you explain what you mean? I think removing the key in the foreach() makes sense. I don't know how it reduces understandability. Can you explain? Thanks.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented

Perhaps it's just me, but I usually like to know what an array is keyed by if I loop trough it. Even though i don't always use the key variable I always define it and give it a meaningfull name. If I ever need to alter something I will instantly know what the array is keyed by without having to guess/look trough documentation/use devel.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Hmm, that goes directly against #2002650: [meta, no patch] improve maintainability by removing unused local variables. I guess it really depends but in many cases I strongly disagree with you, such as:

foreach ($things as $thing) {
}

I don't want to know if its

foreach ($things as $id => $thing) {
}

or

foreach ($things as $weight => $thing) {
}

or whatever. I just want a $thing, do whatever I want with it and be done. Also not using variables can be risky for the reasons explained in the meta issue. If the code were to use the second example in this post, and my $thing actually contains a weighted list of other things I can easily end-up re-using $weight which will lead to bugs.

So, yeah, I really think we should do this, actually.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Assigned: Unassigned » legolasbo
Status: Needs review » Needs work

I've also given this some thought over the past couple of days and i agree with your case in #4. The risk of introducing bugs outweighs the benefit of knowing what a key represents. I'll remove the variables and post a patch shortly.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Assigned: legolasbo » Unassigned
Status: Needs work » Needs review
StatusFileSize
new core-remove-unused-local-variables-views.module-2057157.patch2.77 KB

Attached patch removes the unused variables

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Awesome. I checked that the variables are in fact unused. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Component: other » views.module
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.