2
\$\begingroup\$

I've made an POC about a bitmask implementation in order to replace a huge if/else statement.

Those statements are about strings and I have almost 15 /20 string to test. Depends on values the if/else statement was about 1500 lines of code.

After refactoring I was able to have only 20 objects and a map with all my callbacks in a "handler" class.

The code is working well, but I'm sure it can be improved.


<?php
declare(strict_types=1);

namespace App\Api\Application\Service;

abstract class AbstractValue implements BitwiseCallback
{
    protected $flag;
    public static function getFlag($value): BitwiseCallback
    {
        $class = get_called_class();
        $var = new $class();
        $var->bitValue = $value ? $var->flag : 0;
        return $var;
    }
    protected $bitValue;
    public function bitValue(): int
    {
        return $this->bitValue;
    }
}

<?php
declare(strict_types=1);
namespace App\Api\Application\Service;

interface BitwiseCallback
{
    static function getFlag($value): BitwiseCallback;
    public function bitValue(): int;
}

<?php
declare(strict_types=1);
namespace App\Api\Domain\ValueObject;

use App\Api\Application\Service\VarA;
use App\Api\Application\Service\VarB;
use App\Api\Application\Service\VarC;
use App\Api\Application\Service\VarX;

/**
 * Object Calisthenics or how to use lookup map and biwise map to refactor a huge if statement.
 * Class Test
 * @package App\Api\Domain\ValueObject
 */
class Test
{
    public function aMethod($values)
    {
        $a = VarA::getFlag($values[0]);
        $b = VarB::getFlag($values[1]);
        $c = VarC::getFlag($values[2]);

        $x = VarX::getFlag($values[5]);
// and so on

        $flags = $a->bitValue() | $b->bitValue() | $c->bitValue();
        $map = [
            1 => function() use ($a) { $a->doStuff(); },
            3 => function() use ($a, $b) { return $a->doStuff() . $b->doOtherStuff() . $a->toStuffWithB($b);},
            5 => function() use ($a, $c) { return $c->doStuff() . $b->doOtherStuff() . $c->toStuffWithB($b);},

            // and so on
            35 => function() use ($a, $b, $x) { return $x->sendEmailToAdmin($a, $b) ;}
            // and so on
        ];
        $result = $map[$flags];
        return $result();
    }
}

VarA / VarB / VarC/ and so on are like this

<?php
declare(strict_types=1);
namespace App\Api\Application\Service;
class VarA extends AbstractValue
{
    protected $flag = 1;

    public function doStuff()
    {
        return "This is a correct answer ...";
    }

    public function toStuffWithB($b)
    {
        return ' and you earned a bonus point !';
    }
}
<?php
declare(strict_types=1);
namespace App\Api\Application\Service;

use App\Api\Application\Service\VarA;
use App\Api\Application\Service\VarB;

class VarX extends AbstractValue
{
    protected $flag = 32;

    public function sendEmailToAdmin(VarA $a, VarB $b)
    {
// do stuff with varB and varA because VarX has been triggered
        return "We correctly aknowledge your answer, you'll be notify soon by our customer services";
    }
}

I can't show the real code with the proper names but the implementation remains the same. But' the whole thing is about a quiz with a custom baseline to show depends on answers, each stuff to do are very different depends on the bits (store data to a file / send notification to admin / send a mail about an answer given and so on).

So basically my class that handle the callback map is quite huge.

$map = [
            1 => function() use ($a) { $a->doStuff(); },
            3 => function() use ($a, $b) { return $a->doStuff() . $b->doOtherStuff() . $a->toStuffWithB($b);},
            5 => function() use ($c, $b) { return $c->doStuff() . $b->doOtherStuff() . $c->toStuffWithB($b);}
            // and so on
        ];

this part is quite ugly (around 200 lines of code in my code) with a lot of context and $options.

\$\endgroup\$
  • \$\begingroup\$ Must $flag of every children of AbstractValue be different to each other? Does every VarX class have doStuff(), doOtherStuff() and toStuffWithB() methods? Or in other words, arent those methods meant to be abstract methods of AbstractValue? Does any of the mentioned methods need to do anything with $flag or $bitValue properties? Btw constructors cannot return values. \$\endgroup\$ – slepic Nov 7 at 11:32
  • \$\begingroup\$ Also how is the index of $map related to which vars are in the use() block of the value callback? I thought it would be based on which flags are part of the index. Like 1 => $a because flag of VarA is 1, 3 => $a,$b because flags VarB is 2 and 1+2=3, but 5 => $c,$b yields 2+4=6, and they are in reverse order (not that it matters), but it confuses me, is there any relation at all? \$\endgroup\$ – slepic Nov 7 at 11:37
  • \$\begingroup\$ @slepic $flag are different those are bits of the bitmask (1/2/4/8/16/32...) the $map uses those as an index to unsure that values are filled or not (and make the right calls), you've right about constructors This could be a basic method it was for the snippet ;) tell me if I forgot something \$\endgroup\$ – Hooli Nov 7 at 12:55
  • \$\begingroup\$ And the methods? are they shared across all VarX classes? and why is there 5 => $c,$b then? shouldn't it be $a,$c? And btw it would really help to show some more details, at least another VarX class to see their commons and differences. \$\endgroup\$ – slepic Nov 7 at 13:03
  • 1
    \$\begingroup\$ Please don't change the code in the questions after an answer has been posted, codereview.stackexchange.com/help/someone-answers \$\endgroup\$ – pacmaninbw Nov 7 at 13:22
2
\$\begingroup\$

As implied from the comments under OP, it means that VarA is completly unrelated to VarB, they do completly different things.

If that is true, then it is all entirely obscured way of saying that based on some integer you need to do some arbitrary stuff. Arbitrary except that in all cases it returns a string.

Then all you need is encapsulate each callback in an interface:

interface StringGenerator
{
  public function getTheString(): string;
}

Whether each implementation has the right dependencies that arise from bits that are set in the flags which drive which StringGenerator implementation is used is an implementation detail.

class GeneratorABX implements StringGenerator
{
   private $serviceA;
   private $serviceB;
   private $serviceX;

   public function __construct($serviceA, $serviceB, $serviceX)
   {
     $this->serviceA = $serviceA;
     $this->serviceB = $serviceB;
     $this->serviceX = $serviceX;
   }

   public function getTheString(): string
   {
      return $this->serviceA->doStuff() . $this->serviceB->doOtherStuff() . $this->serviceX->toStuffWithB($this->serviceB);
   }
}

Use a DI container to handle the instantiation of all the implementations of StringGenerator.

function setupContainer(): \Psr\Container\ContainerInterface
{
  $container = new ContainerBuilder();
  $container->addService('serviceA', ServiceA::class);
  $container->addService('serviceB', ServiceB::class);
  $container->addService('serviceX', ServiceX::class);
  //...
  $prefix = 'stringGenerator';
  $container->addService($prefix . '1', GeneratorA::class);
  $container->addService($prefix . '3', GeneratorAB::class);
  $container->addService($prefix . '5', GeneratorAC::class);
  $container->addService($prefix . '35', GeneratorABX::class);

  $container->addService('test', Test::class, ['prefix' => $prefix]);
  return $container->buildContainer();
}

To compute flags from array of bools you can use this:

private static function getFlags(bool ...$values): int
{
  $flags = 0;
  $flag = 1;
  foreach ($values as $value) {
    if ($value) {
      $flags |= $flag; // or $flags += $flag;
    }
    $flag = $flag << 1; // or $flags *= 2;
  }
  return $flags;
}

And to get the right generator:

private function getGenerator($flags): StringGenerator
{
  return $this->container->get($this->prefix . $flags);
}

Final result:

class Test
{
  private $container;
  private $prefix;

  public function __construct(\Psr\Container\CotainerInterface $container, string $prefix)
  {
     $this->container = $container;
     $this->prefix = $prefix;
  }

  public function aMethod($values)
  {
     $flags = self::getFlags(...$values);

     return $this->getGenerator()->getTheString();
  }

  // and the private methods mentioned earlier
}

And call it like this:

$container = setupContainer();
$result = $container->get(Test::class)->aMethod($values);

EDIT: Altogether it is a bit analogous to a router. Router is first setup with a bunch of routes. Upon request, the router choses which controller is to be invoked based on the request and the set of routes. Then the appropriate controller is invoked and response is returned to the caller. The Test class is the router. The setup is done via DI container. The StringGenerators are the controllers. The flags are the request. And the resulting string is the response.

EDIT2: To avoid having a lot of StringGenerator implementations, we can instead have one method per each of the generators. Let me show a way how to do that:

class Test
{
  private $map = [
    1 => ServiceA::class,
    2 => ServiceB::class,
    32 => ServiceX::class,
    // ...
  ];

  private $container;

  public function __construct(\Psr\Container\ContainerInterface $container)
  {
    $this->container = $container;
  }

  public function aMethod(array $values): string
  {
    $services = $this->getServices(...$values);
    return $this->getTheString($services);
  }

  private function getServices(bool ...$values): array
  {
    $services = [];
    $flag = 1;
    foreach ($values as $value) {
      if ($value) {
        $services[$flag] = $this->container->get($this->map[$flag]);
      }
      $flag = $flag << 1; // or $flags *= 2;
    }
    return $services;
  }

  private function getTheString(array $services): string
  {
    $flags = \array_sum(\array_keys($services));
    $method = 'generate' . $flags;
    if (!method_exists($this, $method)) {
      throw new Exception('invalid combination of flags ' . $flags);
    }
    return $this->$method(...array_values($services));
  }

  private function generate1(ServiceA $a): string {}
  private function generate2(ServiceB $b): string {}
  private function generate35(ServiceA $a, ServiceB $b, ServiceX $x): string {}
}

function setupContainer(): \Psr\Container\ContainerInterface
{
  $container = new ContainerBuilder();
  $container->addService(ServiceA::class);
  $container->addService(ServiceB::class);
  $container->addService(ServiceX::class);
  //...
  return $container->buildContainer();
}

$container = setupContainer();
$test = new Test($container);
$result = $test->aMethod($values);

Or the same in a more general, more SOLID, although more verbose version:

interface ServiceProviderInterface
{
  public function getServices(bool ...$values): array;
}

class Services implements ServiceProviderInterface
{
  private $map = [
    1 => ServiceA::class,
    2 => ServiceB::class,
    32 => ServiceX::class,
    // ...
  ];

  private $container;

  public function __construct(\Psr\Container\ContainerInterface $container)
  {
    $this->container = $container;
  }

  public function getServices(bool ...$values): array
  {
    $services = [];
    $flag = 1;
    foreach ($values as $value) {
      if ($value) {
        $services[$flag] = $this->container->get($this->map[$flag]);
      }
      $flag = $flag << 1; // or $flags *= 2;
    }
    return $services;
  }
}

interface StringGeneratorInterface
{
  public function getTheString(array $services): string;
}

class Methods
{
  public function generate1(ServiceA $a): string {}
  public function generate2(ServiceB $b): string {}
  public function generate35(ServiceA $a, ServiceB $b, ServiceX $x): string {}
}

class StringGenerator implements StringGeneratorInterface
{
  private $methods;

  public function __construct(object $methods)
  {
    $this->method = $methods;
  }

  public function getTheString(array $services): string
  {
    $flags = \array_sum(\array_keys($services));
    $method = 'generate' . $flags; // could be a method name inflector responsibility
    if (!method_exists($this->methods, $method)) {
      throw new Exception('invalid combination of flags ' . $flags);
    }
    return $this->methods->$method(...array_values($services));
  }
}

class Test
{
  private $services;
  private $generator;

  public function __construct(ServiceProviderInterface $services, StringGeneratorInterface $generator)
  {
     $this->services = $services;
     $this->generator = $generator;
  }

  public function aMethod(array $values): string
  {
    $services = $this->services->getServices(...$values);
    return $this->generator->getTheString($services);
  }
}

function setupContainer(): \Psr\Container\ContainerInterface
{
  $container = new ContainerBuilder();
  $container->addService(ServiceA::class);
  $container->addService(ServiceB::class);
  $container->addService(ServiceX::class);
  //...
  $container->addService(Services::class);
  $container->addService(Methods::class);
  $container->addService(StringGenerator::class, ['methods' => Methods::class]);
  $container->addService(Test::class);
  return $container->buildContainer();
}

$container = setupContainer();
$result = $container->get(Test::class)->aMethod($values);
```
\$\endgroup\$
  • \$\begingroup\$ So does this mean that you need a class (such as class GeneratorABX) for each possible combination of codes as well as the classed already defined (ServiceA)? \$\endgroup\$ – Nigel Ren Nov 7 at 15:14
  • \$\begingroup\$ Well actualy no. There are more ways. You can also have just one class with a lot of methods. Methods like public function generatorABX(): string;. Let me edit and show that way as well. Anyway you are correct, ServiceA is analogue to your VarA except it knows nothing about flag and has no common parent with ServiceB. \$\endgroup\$ – slepic Nov 7 at 15:17
  • \$\begingroup\$ Isn't this the same as the bit where they are saying there are already 200 lines of the various entries in the $map - which is where your controller comes in. \$\endgroup\$ – Nigel Ren Nov 7 at 15:18
  • \$\begingroup\$ @NigelRen you are sort of right. but it at least removes the coupling of the Test class and decesion which services are used. And coupling of the services from which flag they are assigned to is also removed. Neither of the coupling was necesary in the given use case, i believe. But if there is 200 scenarios you cant simply expect to get rid of 200 "objects", either it be closures, methods, or classes. Depending on their complexity either may be prefered... \$\endgroup\$ – slepic Nov 7 at 15:29
  • \$\begingroup\$ @NigelRen added the version where each generator is just a method... \$\endgroup\$ – slepic Nov 7 at 16:01
0
\$\begingroup\$

I know this isn't a fancy OO solution with controllers and value objects, but this (IMHO) is a straight forward and simple solution which does exactly what it says on the tin.

One of the big problems I have is that you almost seem to be cramming in loads of OO for very little gain. You have 20 classes, one for each variable with some processing. Now if that processing was specific to that variable and it sometimes can be fairly complex I could understand, but especially in your X class (I know this is just an example) you do something with both classes A and B - so firstly why is it in class X and not class A ( AB is in A ), but it also adds cross dependency of classes which doesn't fit with OO principles.

In my version, the basic concept is the same, convert the sequence of answers to a bit mask and then do something based on that bitmap, but in this case the things it calls are firstly just basic functions. You can tune them to do whatever you want which allows you to build the OO structure based on real things (like entities, email etc.)...

// Function names are doStuffn - where n is the mask for the option
function doStuff1() { 
    return "a"; 
}
function doStuff2() {
    return "b";
}
function doStuff3() {
    return "ab";
}
function doStuff5($values) {
    // Call the method to do whatever option 1 does, plus add the value
    // from option 1 and some text.
    return doStuff1().$values[0]."ac";
}

Then the part which calls these...

// Calculate mask
$mask = 0;
$bit = 1;
foreach ( $values as $answer ) {
    if ( $answer )  {
        $mask += $bit;
    }
    $bit <<= 1;
}

// Call the function based on the mask
$fn = "doStuff".$mask;
if ( function_exists($fn) ) {
    echo $mask . "=>" . ($fn($values));
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.