fzaninotto / Faker

Faker is a PHP library that generates fake data for you
MIT License
26.78k stars 3.59k forks source link

Documentation and implementation issues with "optional" method #1227

Open tremby opened 7 years ago

tremby commented 7 years ago

Here's the optional method:

    /**
     * Chainable method for making any formatter optional.
     *
     * @param float|integer $weight Set the probability of receiving a null value.
     *                              "0" will always return null, "1" will always return the generator.
     *                              If $weight is an integer value, then the same system works
     *                              between 0 (always get false) and 100 (always get true).
     * @return mixed|null
     */
    public function optional($weight = 0.5, $default = null)
    {
        // old system based on 0.1 <= $weight <= 0.9
        // TODO: remove in v2
        if ($weight > 0 && $weight < 1 && mt_rand() / mt_getrandmax() <= $weight) {
            return $this->generator;
        }
        // new system with percentage
        if (is_int($weight) && mt_rand(1, 100) <= $weight) {
            return $this->generator;
        }
        return new DefaultGenerator($default);
    }

The docs just really need a rewrite, and I think the implementation does too. It's confusing in a number of ways.

Personally I think expecting a float from 0.0 to 1.0 is the best way to implement this -- this sort of number is much more natural in programming than a number from 0 to 100. (But annoyingly, the float style isn't supported on the boolean generator.)

IlCallo commented 7 years ago

I must agree on a point: if a 1.0 is given, I'd expect the generator, I got 1% instead and lost half a day asking myself why it did not worked as expected (using it under Laravel, i had not PHPDocs attached). Also, numbers between 0 and 1 are really more natural in coding environment to express percentage. Moreover, in this way if i want something to appear the 0.X% of time, I must stick to old style and cannot replicate it with new style (because the first if catches the value). I can understand that it's only until the release of v2, but I can't see the reasons behind the change