Boolean String Normalizer

Question

How can the normalize() method below be improved? I've extracted the functionality from a project I'm working on into a simple class for the purposes of this question.

The method takes a string input and compares it to a set of 'truthy' and 'falsey' strings, returning true or false respectively if a case insensitive match is found. The truthy and falsey strings are mutable so cannot be hard coded into the method.

If the $nullable property is set to true and no match is found, null is returned. Otherwise false is returned.

I feel as though I may be missing a trick by looping through the array and calling strcasecmp twice for each iteration.

<?php

class BooleanNormalizer
{
    /** @var array */
    public $binaries = [
        'yes'     => 'no',
        '1'       => '0',
        'on'      => 'off',
        'enabled' => 'disabled',
    ];

    /** @var bool */
    public $nullable = false;

    /**
     * @param string $value
     *
     * @return bool|null
     */
    public function normalize($value)
    {
        foreach ($this->binaries as $truthy => $falsey) {
            if (strcasecmp($value, $truthy) === 0) {
                return true;
            }

            if (strcasecmp($value, $falsey) === 0) {
                return false;
            }
        }

        return ($this->nullable) ? null : false;
    }
}

Usage:

$normalizer = new BooleanNormalizer();

$normalizer->nullable = true;
$normalizer->binaries = ['enabled' => 'disabled'];

$normalizer->normalize('enabled'); // returns true
$normalizer->normalize('disabled'); // returns false
$normalizer->normalize('blah'); // returns null

Mike Brant · Accepted Answer · 2017-05-01 19:45:49Z

I guess foremost, I question the mutability of this class. Should one really be able to change the value of $binaries?

I very much question the optional "nullable" thing here. First of all, name-wise this doesn't seem to make sense as typically when you think of something as "nullable" it means it is a variable, property, field, etc. that is allowed to be set to null. You are not setting anything here, so the terminology seems weird. Perhaps it should be called $returnFalseOnMissingValue or something that more directly ties it to the return behavior of the normalize() method.

I would however get rid of it, as it will become a source of fragility to calling code, as in some cases (including default case) your calling code would not be able to distinguish a false "hit" result from a false "miss" result. Let the calling code deal with converting null to false if it needs to send false up the call stack. Don't obfuscate the result right here in this class which is designed to own this determination.

Why use strcasecmp()? This seems an odd choice vs. just casting $value to lowercase for direct comparison. i.e.

$value = strtolower($value);
if ($value === $truthy) return true;
if ($value === $falsey) return false;
return null;

I also don't understand the lookup mechanism and comparison mechanisms here. It seems obscure upon first read to have key/value positioning determine how the string is mapped to true/false. I think I would be more explicit like:

protected $mappings = [
    'yes' => true,
    'no' => false,
    '1' => true,
    '0' => false,
    'on' => true,
    'off' => false;
    'enabled' => true,
    'disabled' => false
];

And have your comparison method simply do something like:

public function normalize($value) {
    $value = strtolower($value);
    if(!array_key_exists($value, $this->mappings) return null;
    return $this->mappings[$value];
}

If you truly need to keep the concept of true/value pairs for some reason not shown in this code, then perhaps a structure like:

protected $mappings = [
    'yes' => ['boolean' => true, 'inverse' => 'false'],
    'no' => ['boolean' => false, 'inverse' => 'true'],
    ...
];

This prevents you from having to iterate through the $binaries values every time you want to do a lookup like you are currently doing. The name $binaries also seems odd here as that typically has a much different meaning when talking about writing code.

I would suggest that 'true' and 'false' should be strings in your default mapping of boolean values.

Does this class really need a concrete instantiation?

Consider adding some parameter validation anytime you have public methods. Ideally this could be a combination of type hinting and/or variable inspection to make sure you are being passed a valid parameter. What happens now you if your method is passed an integer, array, object, empty string, etc.?

Thank you for the comprehensive answer. For context, this functionality was extracted from a CSV file reader library and is used to format/normalize an individual cell's contents. Because it's library code, users need to be able to specify the different binary pairs that could appear in the file being imported. The nullable property exists because a cell (field) value could well be null if it is not true/false. 'true' and 'false' are included in my default mappings, I just didn't include them in this sample. — Andy
– Andy, Commented May 2, 2017 at 12:24

Andy · Accepted Answer · 2017-05-01 17:38:26Z

A value => boolean map could be used instead of a truthy => falsey map so that the foreach loop and calls to strcasecmp() can be replaced with one isset() check.

You could add a property to the class for case sensitivity and lower case the values when it is false:

<?php

class BooleanNormalizer
{
    /** @var array */
    public static $defaultBinaries = [
        'yes'     => 'no',
        '1'       => '0',
        'on'      => 'off',
        'enabled' => 'disabled',
    ];

    /** @var bool */
    public $nullable;
    /** @var bool */
    public $caseSensitive;
    /** @var array */
    private $values = [];

    public function __construct($nullable = false, $caseSensitive = false, $binaries = null)
    {
        $this->nullable      = $nullable;
        $this->caseSensitive = $caseSensitive;

        if (func_num_args() === 2) {
            $binaries = static::$defaultBinaries;
        }

        foreach ($binaries as $truthy => $falsey) {
            $this->addBinary($truthy, $falsey);
        }
    }

    /**
     * @param string $truthy
     * @param string $falsey
     */
    public function addBinary($truthy, $falsey)
    {
        if (!$this->caseSensitive) {
            $truthy = mb_strtolower($truthy);
            $falsey = mb_strtolower($falsey);
        }

        $this->values[$truthy] = true;
        $this->values[$falsey] = false;
    }

    /**
     * @param string $value
     *
     * @return bool|null
     */
    public function normalize($value)
    {
        if (!$this->caseSensitive) {
            $value = mb_strtolower($value);
        }

        if (isset($this->values[$value])) {
            return (bool)$this->values[$value];
        }

        return ($this->nullable) ? null : false;
    }
}

BenKoshy · Accepted Answer · 2017-05-03 04:38:42Z

Update: based on your clarified question I have updated my answer

Here's what I whipped up:

Basically i have created a hash (which is basically a dictionary using c# language, or a set of key values pairs). You can iterate through the key value pairs to get what you want. But I don't have a good idea of what you are actually trying to do - it suggests to me that the code could be refactored to be better from an overall viewpoint, rather than merely focusing on this boolean string normalizer class. i hope this helps. @MikeBrant makes some points worth considering too.

you can set the hash/dictionary values for example like below:

@n.binaries = {true => ['enabled', 'yes', 'on', '1'], false => ['disabled', 'no', 'off', '0']}

Then you just iterate through the values and if there's a match, then you return the appropriate key.

Normalizer.rb

class Normalizer
    attr_accessor :binaries

    def normalize(input_value)
        # iterates through the binaries and 
        # returns the appropriate true/false values            

        binaries.each do |boolean_value_key, array|
            array.each do |input|
                if input == input_value                 
                    return boolean_value_key
                end
            end
        end     
        return nil      
    end
end

Normalizer_tests.rb

# magic_ball.rb
require 'minitest/autorun'
require_relative 'normalizer'

class NormalizerTest < Minitest::Test

    def setup
        @n = Normalizer.new()
        @n.binaries = {true => ['enabled', 'yes', 'on', '1'], false => ['disabled', 'no', 'off', '0']}
    end

  def test_returns_true_for_enabled    
    result = @n.normalize('enabled')
    assert_equal true, result
  end

  def test_returns_false_for_off
    result = @n.normalize('off')
    assert_equal false, result
  end

   def test_returns_nil_for_blah
    result = @n.normalize('blah')
    assert_equal nil, result
  end


end

@MartinR good point - but its just syntax: the concept is what i think OP is looking for. — BenKoshy
– BenKoshy, Commented May 1, 2017 at 13:45
Thanks for your answer. I've edited my question to explain what the method actually does. I can't hard code the truthy and falsey values into the method because they are mutable. — Andy
– Andy, Commented May 1, 2017 at 13:57

Stack Exchange Network

Boolean String Normalizer

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Boolean String Normalizer

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions