Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:109048
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
In-Reply-To: <DM5PR07MB30674A0E48CA751CD00B9F46F9F80@DM5PR07MB3067.namprd07.prod.outlook.com>
Date: Sun, 15 Mar 2020 22:05:37 -0400
Cc: Internals <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <C2BCD046-A200-4E01-B269-3FC0F9D7E48F@newclarity.net>
References: <DM5PR07MB3067D2DEAD4B3C85FBDFD559F9F80@DM5PR07MB3067.namprd07.prod.outlook.com>
 <fb548f2a-0f0d-4dbe-8b69-b4c94501ecc4@Mikes-iPad-Pro>
 <DM5PR07MB30678914A5A748371901797FF9F80@DM5PR07MB3067.namprd07.prod.outlook.com>
 <30C23964-6E52-4B2B-8947-8BD3E9E46354@newclarity.net>
 <DM5PR07MB30674A0E48CA751CD00B9F46F9F80@DM5PR07MB3067.namprd07.prod.outlook.com>
To: tyson andre <tysonandre775@hotmail.com>
Subject: Re: [PHP-DEV] RFC idea: Block scoped variables with "let $x =  expr"
From: mike@newclarity.net (Mike Schinkel)

> On Mar 15, 2020, at 7:42 PM, tyson andre <tysonandre775@hotmail.com> =
wrote:
>=20
> Hi Mike,
>=20
>> What I am about to say is ironic since I too often feel people are =
trying to enforce their view of coding on the world,
>> but I do not think we want to give people tools to make it easier to =
work with long functions/scripts.
>=20
> Blocked-scoped variables is a tool which can make short code and long =
code more readable.
> Making it easier to work with long functions is an example of a =
benefit of having block-scoped variables,
> not the main goal of this RFC idea.
>=20
> =
https://github.com/getify/You-Dont-Know-JS/blob/1st-ed/scope%20%26%20closu=
res/ch3.md#blocks-as-scopes
> goes into some of the other reasons to use block-scoped variables in =
JS
>=20
> - Many languages other than JavaScript/PHP support Block Scope,
>  and so developers from those languages are accustomed to the mindset, =
whereas those who've primarily only worked in JavaScript may find the =
concept slightly foreign.
> - Block scope is a tool to extend the earlier "Principle of Least =
Privilege Exposure"
>  from hiding information in functions to hiding information in blocks =
of our code.
> - Developers may prefer to check themselves against accidentally =
(re)using variables outside of their intended purpose,
>  such as being issued an error about an unknown variable if you try to =
use it in the wrong place.
>  Block-scoping (if it were possible) for the `i` variable would make =
`i` available only for the for-loop,
>  causing an error if `i` is accessed elsewhere in the function.
>  This helps ensure variables are not re-used in confusing or =
hard-to-maintain ways.

I hear you.  Personally, I do not find any of those compelling enough to =
outweigh the concerns I have already raised.

But again, it is just my opinion and I respect your right and anyone =
else's to evaluate the pros and cons differently than I do.

>=20
>> I would be very interested in seeing business logic coded into a long =
function that literally cannot be made better by refactoring into =
multiple smaller functions.
>> If you have some, please do post to a Gist and share it to see if it =
in fact it cannot be refactored and made "better."
>=20
> One example is if PHP itself is used for HTML rendering in top-level =
statements - pages or components of a page may be spread out across =
hundreds of lines.
> This could be moved to a different templating engine for PHP instead =
of raw PHP,
> but the refactoring would be time-consuming and error-prone.

Since you did not provide any specific examples I will overlook this...

> Separately from HTML rendering, some problem domains are just complex.
> For example, consider the functions such as decodeMPEGaudioHeader in
> =
https://github.com/WordPress/WordPress/blob/master/wp-includes/ID3/module.=
audio.mp3.php
> (I'm not involved in that project or file)

Ironically, I work primarily with WordPress, and while I do so I find =
much of the code in WordPress core to be bordering on completely awful, =
and this code is no exception.

Looking at the decodeMPEGaudioHeader() function it is over 600 lines of =
meandering spaghetti, and a wonderful example of why enabling people to =
make these kind of functions more maintainable is akin to giving addicts =
the strongest, most addictive drugs imaginable. :-)

It would take more than 30 minutes to do a proper refactor, but here is =
just a sample of what could be done in half an hour; from 600+ lines to =
~200:

https://gist.github.com/mikeschinkel/8bb52534836e3cd19eee7d4f3f5fa9fd


> You could split it up into multiple helper functions, but instead of =
hundreds of lines in one function for a complicated file format,
> you'd have to jump across even more abstractions/lines to know where =
XYZ was set.

I would argue that it is much harder to grok the entirety of the audio =
file by having to read a 600+ line function than if the main function =
called different functions that each handled a different part of the =
file.=20

In refactoring the above, I found clear and distinct areas to separate =
for LAME encoding and different variable bit rate encoding types.  Also, =
this function is begging to be refactoring to use a variety of object =
classes, even if the output is covered to arrays for backward =
compatibility.

Instead of making it harder to understand the file format, breaking it =
out to multiple functions that mirror that different aspects of process =
a file will allow the developer to see the big picture =E2=80=94 which =
is currently impossible with a 600 line function =E2=80=94 and then to =
zoom into specific program functions when they want to understand more =
detail about a specific area.

> Fully refactoring that might not make ever make business sense,
> but there is a benefit to having the ability to mark if a variable is =
actually block-scope or used elsewhere
> (e.g. to have a better idea of if refactoring to a different function =
is safe)

I totally get and agree with the concern about refactoring not making =
business sense.  If you don't *need* to touch the code, don't touch the =
code.

But if you are going to modify the file to add block level declarations =
then you can just as easily refactor the area you would modify into its =
own function. Leaving as one large function means that you will likely =
never fully understand the patterns at play, and thus will never be able =
to simplify maintenance.  And will almost certainly have to play =
whack-a-mole with bugs each time someone modifies it.

One thing is certain; you cannot reliably unit-test a long function like =
that; there is just too much going on to test in that function.

This type of long function is what has plagued my current client and has =
caused them to spend about 80% of their developer time fixing bugs =
rather than devoting most of their time to build new features to help =
them grow their business (they brought me on specifically to fix that =
problem, so seeing you trying to empower developers to create more =
spaghetti like that made me feel the need to speak out. But again, it's =
just my opinion.)

>> Given the nature of long functions, I think the awkward syntax here =
provides a nice disincentive to writing overly long functions.
>>=20
>> But as I said, the closure does exist in case someone absolutely =
needs to control
>> the scope of a block within an existing long function, such as the =
1400 line function I've been refactoring for months now...
>=20
> Also, closures aren't a convenient substitute for everything the local =
scope can do for other reasons.
> E.g. modifying by reference will mask issues with undefined or =
possibly undefined variables when modifying values outside of the =
closure scope.
>=20
> ```
> function compute_product($values) {
>    // Other lines omitted
>    foreach ($values as $value) {
>        (function() use (&$product, $value) {
>            $letVar =3D $value + 1;
>            // Other lines omitted
>            $product *=3D $letVar;
>        })();
>    }
>    return $product;
> }
> ```
>=20
> This has the bug that $product was never defined, but no notices are =
emitted because $product gets set to null
> when the reference is taken by reference, and php currently doesn't =
warn about multiplying by null.
>=20
> Using an actual `let $var =3D $value + 1`, this would get the notice =
for the variable being undefined.

This example is too contrived for me to determine if there is an easy =
alternative coded it in a clearer manner that exist that does not have =
the concerns you mention.  At first blush I would have the closure =
return a value and then do the multiplication outside the closure which =
solves your stated issue, but then the closure really does nothing, or =
at least nothing without the omitted lines included.

Still, I don't argue that you *should* use closures instead of =
refactoring to additional functions, just that they are there to use in =
case you are determined not to refactor into multiple functions.

#fwiw

-Mike