Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121335
Message-ID: <ba179d83-362b-46b6-859d-a1cc07e16ec7@bastelstu.be>
Date: Mon, 16 Oct 2023 14:57:47 +0200
MIME-Version: 1.0
Content-Language: en-US
To: internals@lists.php.net
References: <CAL-Ucw7H4zcbnhUJHHPi9=KX9NtPEVKiCOyNz+TTJfdwkzLRTw@mail.gmail.com>
 <c4b40b7a-aa8b-470f-ae07-1485c010d907@bastelstu.be>
 <A14E0D76-9EB2-4881-8F9C-4A2848EFCCFF@cschneid.com>
In-Reply-To: <A14E0D76-9EB2-4881-8F9C-4A2848EFCCFF@cschneid.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Better name for method Randomizer::nextFloat()
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

On 10/16/23 10:34, Christian Schneider wrote:
> Am 15.10.2023 um 19:24 schrieb Tim Düsterhus <tim@bastelstu.be>:
>> Making getFloat(float $min = 0.0, float $max = 1.0, IntervalBoundary $boundary = IntervalBoundary::ClosedOpen) would seemingly make it legal to call ->getFloat(0.5), which I consider to be worse than nextFloat().
> 
> While I understand that you find getFloat(0.5) undesirable I would not consider it illegal, e.g. getFloat(max:5).
> Additionally I would consider getInt(max:100) to be a very valid usage too.

I don't, because I prefer explicit calls. If the upper bound is 
specified explicitly, the lower bound should be as well. In fact the 
`0,` is even shorter than spelling out `max:`.

>> Side note: The implementation of nextFloat() is much more efficient than that of getFloat(0.0, 1.0) and the output will differ even for the same seeded engine. The domain of legal return values is identical.
> 
> This sounds like an implementation detail as I'm pretty sure you could switch to that fast path easily enough if neither of the arguments were given.

Yes, that's why it's a side note.

> Side-note: The case of nextInt() is a bit trickier and strikes me as a bit of a weird API and I'd consider dropping it too: Having the max value depend on the Randomizer engine makes the generated values unstable in regards to switch the Engine. And if I read the documentation correctly then it only generates 32 bit values (or should it be 31 bits as it returns positive values only?). I think it would be clearer to require the Engine to provide at least 4 bytes and then specify the default max value of getInt() to be 2^31, optionally overridable with something up to PHP_INT_MAX.

Yes, Randomizer::nextInt() is terrible. Let me provide the context why 
it exists like it exists:

1. The Randomizer was designed to be drop-in compatible with the 
existing randomness functionality (mt_rand, array_rand, shuffle, 
strshuffle) when combining it with the Mt19937 engine.

2. Thus mt_rand is equivalent to Randomizer::getInt().

3. It is legal to call mt_rand() without parameters.

4. This behavior was carried over into getInt().

5. During the final review, it was noticed that this is an overloaded 
function and a decision was made not to do this, because overloaded 
functions are terrible.

6. Thus the getInt()-without-parameters call was split into nextInt().

I think that the goal of making the new API a drop-in replacement has 
been a useful goal to make it easier to migrate and I assume that's why 
no one questioned whether retrieving "an arbitrary positive integer" is 
even useful in the first place. While writing the documentation I first 
realized how useless that is, because I was unable to write something 
useful for Randomizer::nextInt(), but at that point it was way too late. 
Hindsight is 20/20.

Given that knowledge, is naming `nextFloat()` like `nextInt()` a 
mistake? I'm not sure. It was the obvious naming choice to me back when 
I wrote the RFC a year ago and no one pointed out how the name was not 
ideal, not during the RFC discussion phase, nor during the vote or in 
the half year leading up to the feature freeze. Now 6 weeks before the 
gold release, with 4 release candidates released and several blog posts 
written, the method name suddenly is a bad choice?

> So in summary I'd support both adding default values to getInt()/getFloat() as well as dropping nextFloat() in favor of getFloat() (and possible nextInt() with getInt()).
> 

I'd be on board with deprecating nextInt() in PHP 8.4. I'd also be on 
board with creating a better-named alias for nextFloat(), but I believe 
that it should remaing a standalone method, because of its usefulness.

Best regards
Tim Düsterhus