Page 1 of 1

randomString Object Class

Posted: Thu Mar 05, 2009 11:51 pm
by Benjamin
Hey guys, I've transitioned to MVC. I've been able to figure mostly everything out so I haven't asked any questions about it yet. I have made considerable progress in the last 3 weeks.

So working in this new environment requires me to create classes differently. They take a lot longer to write but I can see the benefits.

I created the following class this morning, at the end of a 20 hour educational marathon. I think it can be improved significantly so I'm looking for feedback from you. Please copy and paste the code into your editor and have a look at it. Let me know what you would do differently and why. Even if it's a very minor issue, please let me know. I want to ensure that I write the best code I possibly can and I hope to learn from this.

The class:

Code: Select all

 
<?php
class randomString
{
    /**
     * Contains a string of numbers used in random strings.
     * @var string A string of numeric characters
     * @access private
     */
    private $_numbers;
 
    /**
     * Contains a string of uppercase characters used in random strings.
     * @var string A string of uppercase alpha characters
     * @access private
     */
    private $_upper;
 
    /**
     * Contains a string of lowercase characters used in random strings.
     * @var string A string of lowercase characters
     * @access private
     */
    private $_lower;
 
    /**
     * Contains a string of symbols used in random strings.
     * @var string A string of symbol characters
     * @access private
     */
    private $_symbols;
 
    /**
     * Whether to use numbers in the generated string or not
     * @var bool
     * @access private
     */
    private $_use_numbers;
 
    /**
     * Whether to use uppercase letters in the generated string or not
     * @var bool
     * @access private
     */
    private $_use_upper;
 
    /**
     * Whether to use lowercase letters in the generated string or not
     * @var bool
     * @access private
     */
    private $_use_lower;
 
    /**
     * Whether to use symbols in the generated string or not
     * @var bool
     * @access private
     */
    private $_use_symbols;
 
    /**
     * The length of the string to be generated
     * @var int
     * @access private
     */
    private $_length;
 
    /**
     * The randomly generated string
     * @var string
     * @access private
     */
    private $_random_string;
 
    /**
     * A map of options to their corresponding set method
     * @var array
     * @access private
     */
    private $_option_map;
 
    /**
     * Class constructor
     *
     * @param array $options An array of options
     */
    public function __construct($options = array()) {
        $this->_option_map = array(
            'lower'       => 'setLower',
            'upper'       => 'setUpper',
            'numbers'     => 'setNumbers',
            'symbols'     => 'setSymbols',
            'use_lower'   => 'enableLower',
            'use_upper'   => 'enableUpper',
            'use_numbers' => 'enableNumbers',
            'use_symbols' => 'enableSymbols',
            'length'      => 'setLength',
        );
 
        $this->setDefaults();
        $this->setOptions($options);
 
        return $this;
    }
 
    /**
     * <p>Sets all settings to default for random string creation.</p>
     *
     * <p>Default Settings:</p>
     * numbers:     0123456789<br>
     * upper:       ABCDEFGHIJKLMNOPQRSTUVWXYZ<br>
     * lower:       abcdefghijklmnopqrstuvwxyz<br>
     * symbols:     !"#$%&'()*+,-./:;<=>?@[\]^_`{|}~<br>
     * <br>
     * use_lower:   false<br>
     * use_upper:   true<br>
     * use_numbers: true<br>
     * use_symbols: false<br>
     * 
     * @access public
     */
    public function setDefaults() {
        $this->_numbers         = '0123456789';
        $this->_upper           = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
        $this->_lower           = 'abcdefghijklmnopqrstuvwxyz';
        $this->_symbols         = '!"#$%&\'()*+,-./:;<=>?@[\]^_`{|}~';
 
        $this->_use_lower       = false;
        $this->_use_upper       = true;
        $this->_use_numbers     = true;
        $this->_use_symbols     = false;
 
        $this->_length          = 10;
 
        $this->_random_string   = false;
    }
 
    /**
     * Generates a random string
     *
     * @returns string A random string
     */
    public function create() {
        $charPool = '';
 
        if ($this->_use_lower == true) {
            $charPool .= $this->_lower;
        }
 
        if ($this->_use_upper == true) {
            $charPool .= $this->_upper;
        }
 
        if ($this->_use_numbers == true) {
            $charPool .= $this->_numbers;
        }
 
        if ($this->_use_symbols == true) {
            $charPool .= $this->_symbols;
        }
 
        # subtract 1 because the string offsets start at 0
        $charPoolLen = strlen($charPool) - 1;
        
        $this->_random_string = '';
 
        for ($i = 0; $i < $this->_length; $i++) {
            $this->_random_string .= $charPool[mt_rand(0, $charPoolLen)];
        }
 
        return $this->_random_string;
    }
 
    /**
     *
     * @access public
     */
    public function get() {
        if ($this->_random_string === false) {
            $this->create();
        }
 
        return $this->_random_string;
    }
 
    /**
     * Sets the string of lowercase characters used for creating the random
     * string.
     *
     * @var string $string The string of characters.  The value will be
     * converted to lowercase.
     * @access public
     * @returns An instance of this class.
     */
    public function setLower($string) {
        $this->_lower = strtolower((string) $string);
        return $this;
    }
 
    /**
     * Sets the string of upper case characters used for creating the random
     * string.
     *
     * @var string $string The string of characters.  The value will be
     * converted to uppercase.
     * @access public
     * @returns An instance of this class.
     */
    public function setUpper($string) {
        $this->_upper = strtoupper((string) $string);
        return $this;
    }
 
    /**
     * Sets the string of numerical characters used for creating the random
     * string.
     *
     * @var string $string The string of numbers.
     * @access public
     * @returns An instance of this class.
     */
    public function setNumbers($string) {
        $this->_numbers = (string) $string;
        return $this;
    }
 
    /**
     * Sets the string of symbols used for creating the random string.
     *
     * @var string $string The string of symbols.
     * @access public
     * @returns An instance of this class.
     */
    public function setSymbols($string) {
        $this->_symbols = (string) $string;
        return $this;
    }
 
    /**
     * Sets whether lower case characters will be used when creating the
     * random string
     *
     * @var bool $bool true to enable, false to disable
     * @access public
     * @returns An instance of this class.
     */
    public function enableLower($bool) {
        $this->_use_lower = (boolean) $bool;
        return $this;
    }
 
    /**
     * Sets whether upper case characters will be used when creating the
     * random string
     *
     * @var bool $bool true to enable, false to disable
     * @access public
     * @returns An instance of this class
     */
    public function enableUpper($bool) {
        $this->_use_upper = (boolean) $bool;
        return $this;
    }
 
    /**
     * Sets whether symbols will be used when creating the random string
     *
     * @var bool $bool true to enable, false to disable
     * @access public
     * @returns An instance of this class
     */
    public function enableSymbols($bool) {
        $this->_use_symbols = (boolean) $bool;
        return $this;
    }
 
    /**
     * Sets the length of the generated random string
     *
     * @var int $int The length of the string
     * @access public
     * @returns An instance of this class
     */
    public function setLength($int) {
        $this->_length = (int) $int;
        return $this;
    }
 
    /**
     * Sets class options
     * @var array $options An array of options. defaults, numbers, upper, lower,
     * symbols
     * @access public
     * @returns An instance of this class
     */
    public function setOptions($options) {
        if (!is_array($options)) {
            trigger_error("Argument passed to randomString->setOptions() must be an array", E_USER_ERROR);
            return $this;
        }
 
        foreach ($this->_option_map as $option => $method) {
            if (isset($options[$option])) {
                $this->$method($options[$option]);
                unset($options[$option]);
            }
        }
 
        if (count($options) > 0) {
            foreach ($options as $option) {
                trigger_error(sprintf("%s passed to randomString->setOptions() is not an allowable option", $option), E_USER_ERROR);
            }
        }
 
        $this->_random_string = false;
 
        return $this;
    }
 
    /**
     * Returns the random string and generates one if it has not been created
     * yet
     * @return string A random string
     * @see getString()
     */
    public function __toString() {
        return $this->get();
    }
}
 
 
The unit tests:

Code: Select all

 
<?php
require_once dirname(__FILE__).'/../bootstrap/unit.php';
$t = new lime_test(182, new lime_output_color());
 
/*
 * generate 20 random strings
 * 4 tests on each string: total = 80
 * default settings
 */
 
for($i = 0; $i < 20; $i++) {
    $ran = new randomString();
    $randomString = $ran->get();
 
    $t->comment("randomString() - generated $randomString");
    $t->is(strlen($randomString), 10, 'randomString()->createString() length is 10 characters');
    $t->unlike($randomString, '#[a-z]+#', 'randomString()->createString() does not contain lower case characters');
    $t->like($randomString, '#^[A-Z0-9]+$#', 'randomString()->createString() contains only upper case letters and numbers');
    $t->unlike($randomString, '#[!"\#\$%&\'()*+,-./:;<=>?@[\]^_`{|}~]+#', 'randomString()->createString() does not contain symbols');
}
 
/*
 * generate 20 random strings
 * 5 tests on each string: total = 100
 * turn on symbols and lowercase letters
 * change length to 40
 */
for($i = 0; $i < 20; $i++) {
    $ran = new randomString(array(
            'length'      => 40,
            'use_lower'   => true,
            'use_symbols' => true,
        ));
    $randomString = $ran->get();
 
    $t->comment("randomString() - generated $randomString");
    $t->is(strlen($randomString), 40, 'randomString()->createString() length is 40 characters');
    $t->like($randomString, '#[a-z]+#', 'randomString()->createString() does contain lower case characters');
    $t->like($randomString, '#[A-Z]+#', 'randomString()->createString() does contain upper case characters');
    $t->like($randomString, '#^[a-zA-Z0-9!"\#\$%&\'()*+,\-\./:;\\\=>?@[\]^_`{|}~]+$#', 'randomString()->createString() contains lower, upper, numbers and symbols');
    $t->like($randomString, '#[!"\#\$%&\'()*+,\-\./:;\\\=>?@[\]^_`{|}~]+#', 'randomString()->createString() does contain symbols');
}
 
$ran = new randomString();
$string = $ran->enableLower(false)->enableSymbols(true)->get();
$t->comment("enableLower(false)->enableSymbols(true)->get() - generated $string");
$t->unlike($string, '#[a-z]+#', 'randomStenableLower(false)->enableSymbols(true)->get() does contain lower case characters');
$t->like($string, '#[!"\#\$%&\'()*+,\-\./:;\\\=>?@[\]^_`{|}~]+#', 'enableLower(false)->enableSymbols(true)->get() does contain symbols');
 
 
Usage Examples:

Code: Select all

 
$ran = (string) new randomString();
echo "$ran\n";
 
$ran = (string) new randomString(array('length' => 25));
echo "$ran\n";
 
$ran = new randomString();
$string = $ran->setLength(20)->
        setLower('abcde')->
        setUpper('FGHIJ')->
        setNumbers('123')->
        enableLower(true)->
    get();
 
echo "$string\n";
 
$ran = new randomString(array(
        'length'      => 40,
        'use_lower'   => true,
        'use_symbols' => true,
    ));
$string = $ran->get();
 
echo "$string\n";
 

Re: randomString Object Class

Posted: Fri Mar 06, 2009 1:43 am
by Christopher
I just took a quick look. I might create individual methods to set the various options, and then have your central method that takes an array of options call those methods. You also might want to add the __toString() method to this class.

Re: randomString Object Class

Posted: Fri Mar 06, 2009 2:08 am
by Benjamin
arborint wrote:I just took a quick look. I might create individual methods to set the various options, and then have your central method that takes an array of options call those methods. You also might want to add the __toString() method to this class.
Great idea on the __toString() method. I forgot about that.

I've really been struggling with whether to implement different methods for setting options or use one method that accepts an array of key => values. I'd like to hear the benefits and drawbacks of both.

Re: randomString Object Class

Posted: Fri Mar 06, 2009 10:54 am
by pickle
I'm by no means an MVC maven, so some of my suggestions might go against that paradigm:
  • Why do all your private variables start with _? It's entirely personal preference, but it seems superfluous to me
  • Change your "_str_length" to just "length" (or "_length" if you prefer). It's a property of the "randomString" object, so there's no need to specify "str" again. It's a bit like having an "Orange" object, and having a function called "peelOrange()", when "peel()" would suffice. Actually, since I'm making this point, createString() and getString() should be called create() and get() respectively.
  • Why have a whole function to calculate the values of _lower, _upper, etc? Every time this class is instantiated, it needs to re-calculate those values. Why not just hardcode them in the variable declaration? It would make it much easier to modify in the future as well.
  • I agree with ~arborint - have 1 function for each option.
  • I had to escape a few extra characters in your setOptions function, for checking symbols. I'm not sure if this still does what you want, I just escaped what I needed to to make the page display.

    Code: Select all

    #([^!"\#$%&\'\()\*\+,-\./:;<=>?@[\\\]^_`{|}~]+)#i
  • There's no need to convert charPool to an array in your createString() function

    Code: Select all

    $charPoolLen = strlen($charPool);
           
    $this->_random_string = '';
     
    for ($i = 0; $i < $this->_str_length; $i++) {
        $this->_random_string .= $charPool{mt_rand(0, $charPoolLen)};
    }
  • All your unit tests access createString() directly, not getString()

Re: randomString Object Class

Posted: Fri Mar 06, 2009 3:10 pm
by André D
pickle wrote:There's no need to convert charPool to an array in your createString() function

Code: Select all

$charPoolLen = strlen($charPool);
       
$this->_random_string = '';
 
for ($i = 0; $i < $this->_str_length; $i++) {
    $this->_random_string .= $charPool{mt_rand(0, $charPoolLen)};
}
Pickle is right, you don't need str_split() because you can simply use array-like syntax to access a string by character index. One minor adjustment, however, is that it's better to use square brackets instead of curly braces. Curly braces for string access are being deprecated.

Code: Select all

$this->_random_string .= $charPool{mt_rand(0, $charPoolLen)}; // Deprecated string access by character
$this->_random_string .= $charPool[mt_rand(0, $charPoolLen)]; // Proper string access by character
Also, instead of dictating that the character pool be a combination of numerals, lower-case letters, upper-case letters or symbols, why not simply allow the user of this class to supply his own string with the characters he wants to use? This makes it possible to use some symbols while excluding other symbols that are not safe for URLs or file names, for example. This will also make your class simpler because you'll have fewer options to configure. I do something similar in my FixedBitNotation class, which is also posted here in the Coding Critique forum; check it out.

Re: randomString Object Class

Posted: Fri Mar 06, 2009 5:31 pm
by André D
Just a few thoughts on random token generation, for what it's worth:

I like your method of generating random strings because it produces a very balanced character distribution, no matter how many characters you include in your base alphabet (especially if you implement my earlier suggestion to let the user supply it). This is achieved through your use of mt_rand() for each character. However, there are people who wouldn't generate random strings this way because mt_rand() is not sufficiently random for their needs. I'm not sure what implications the repeated calls to mt_rand() with a small range has for those who are very concerned with generating strong random tokens.

It's an interesting approach: piece together a string one random character at a time. I think the more common approach is to get a large random number, then convert it to a string using a notation (usually hexadecimal) that uses characters that are safe for the random string's intended purpose. Often this is achieved by running random data through a hash function. My favorite approach is to read random data from /dev/urandom and encode it with my FixedBitNotation class.

By way of comparison, your technique has the potential to produce tokens with a well balanced character distribution using any alphabet size. My FixedBitNotation class can only do so with alphabet sizes that are a power of 2 (assuming sufficiently random input). On the other hand, this approach locks in the use of mt_rand() for pseudo-random number generation, while FixedBitNotation separates the concept of random number generation and encoding, so that the user can supply his own random data which he generated however he sees fit.

Different jobs require different tools, and sometimes it comes down to personal preference.

Re: randomString Object Class

Posted: Sun Mar 08, 2009 6:17 pm
by Benjamin
Changes:
  • Added __toString() method
  • Renamed _str_length to _length
  • Renamed createString() to create()
  • Renamed getString() to get()
  • Hardcoded values of _lower, _upper, _number and _symbols
  • Added set methods for each option
  • Added chaining support
  • Unit tests now call get, rather than create, but this does not affect results
  • charPool is not converted to an array anymore, but is still used as one with string offsets

The private variables start with an underscore because this makes them instantly recognizable as private. As far as I understand, this is a common convention. I like being able to set a bunch of options with one method call. So I left that in place. I did however add individual setters for each option. This should be the best of both worlds.

Allowing the user to specify a custom string is something that can easily be added. It's good to be able to just fire up an instance, turn certain character sets on or off and pull a random string without having to worry about creating my own character list. But I can see how someone may want to specify a set of say 123ABC so that the returned string only contains these characters. The class at this point does support that, but you'll have set a few options to get there.

I'm not really concerted about the randomness of the results. Even a 5 character random string of uppercase, lowercase and numbers has 916,132,832 combinations. When your talking about
random strings for email validation, that could definitely be called overkill.

The new code has been saved to my original post.

Re: randomString Object Class

Posted: Mon Mar 09, 2009 4:28 pm
by Benjamin
This stinks, it shouldn't be so long. What's up guys?

Re: randomString Object Class

Posted: Mon Mar 09, 2009 5:09 pm
by Christopher
What stinks? Is your class too long or has no one responded to your last post?

Re: randomString Object Class

Posted: Mon Mar 09, 2009 5:21 pm
by Benjamin
arborint wrote:What stinks? Is your class too long or has no one responded to your last post?
Well, I try to keep things simple and concise. It seems like the class is overly complex for what it does.

The class allows you to generate a random string. You can specify the length, whether uppercase, lowercase, numbers and symbols are used, and you can specify your own character lists.

Should that really require 300~ lines of code?

Re: randomString Object Class

Posted: Tue Mar 10, 2009 12:12 am
by pickle
You know the old paradigm:
Software can be quick, cheap or good. Pick any 2.
The same can be said about capability, simplicity, and clarity. You've chosen it to be capable and clear, so it's not as simple as it could be - which isn't a bad thing at all.

Myself, I would have just made a static function that accepts a string of possible characters and a desired length, but different strokes for different folks - ~Chris Corbyn probably loves this :)

Re: randomString Object Class

Posted: Tue Mar 10, 2009 12:19 am
by Benjamin
Well that's refreshing to hear... Thanks pickle!

Re: randomString Object Class

Posted: Tue Mar 10, 2009 1:27 am
by Christopher
astions wrote:Should that really require 300~ lines of code?
Well now you might want to think about both your approach and design. You thought of a problem and a solution -- and coded it. Now can you think of a simpler solution to the problem? What is causing there to be a lot of code? Do you really need those features or can you get the same result in a simpler way?

Re: randomString Object Class

Posted: Wed Mar 11, 2009 1:45 pm
by Jenk
I think as an implementation, it should be:

Code: Select all

$rs = new RandomString('allowed chars', 20);
echo $rs;
Where 'allowed chars' is the string containing allowed characters, and 20 is the length. I don't think it's necessary to have the allowUpper/allowLower and so forth. Maybe even have a regex as the argument and something like:

Code: Select all

class RandomString {
  private $_regex;
  private $_len;
  private $_string = '';
 
  public function __constructor($regex, $len) {
    $this->_regex = $regex;
    $this->_len = $len;
    $this->generate();
  }
 
  public function generate() {
    $this->_string = '';
    for ($i = 0, $char = chr(mt_rand(32, 236)); $i < $this->_len;) {
      if (preg_match($this->_regex, $char)) {
        $this->_string += $char;
      }
    }
  }
 
  public function __toString() {
    return $this->_string;
  }
}
Performance probably poo, so instead of regex, use in_array() on a string of allowed chars (if strings can be used as arrays to that degree?)