0
\$\begingroup\$

I'm using PHP, I have a class that generates an id based on the calling class, so the id of the class won't change after it has been initialised, now I'm wondering which of my two methods is preferrable.

  • Set the id in the constructor
  • Generate the id when asked for
/* Set  the id in the constructor */

abstract class Badge
{
    protected $activities;
    protected $id;

    abstract public function check();

    public function __construct(array $activities)
    {
        $this->activities = $activities;
        $this->id = $this->getId();
    }

    public function getId(): string
    {
        $class = explode('\\', get_class($this));
        $classWithoutNamespace = array_pop($class);
        $snakeCased = strtolower(preg_replace('/(?<!^)[A-Z]/', '_$0', $classWithoutNamespace));

        return "{$snakeCased}_{$this->target}";
    }

    public function toArray(): array
    {
        return [
            'id' => $this->id,
        ];
    }
}
/* Generate the id when asked for */

abstract class Badge
{
    protected $activities;

    abstract public function check();

    public function __construct(array $activities)
    {
        $this->activities = $activities;
    }

    public function getId(): string
    {
        $class = explode('\\', get_class($this));
        $classWithoutNamespace = array_pop($class);
        $snakeCased = strtolower(preg_replace('/(?<!^)[A-Z]/', '_$0', $classWithoutNamespace));

        return "{$snakeCased}_{$this->target}";
    }

    public function toArray(): array
    {
        return [
            'id' => $this->getId(),
        ];
    }
}

```
\$\endgroup\$
1
  • \$\begingroup\$ Your code seems to reference undefined member property $this->target. Also please provide some code that shows how the class is used. \$\endgroup\$
    – slepic
    Aug 26, 2020 at 12:42

2 Answers 2

4
\$\begingroup\$

I would go with lazy loading. Generate the id when requested

private $id;

public function getId()
{
    if ($this->id === null) {
        $this->id = //your logic to generate the id here
    }
    return $this->id;
}

and don't use anywhere else $this->id even inside the class. Use $this->getId() everywhere.

using $this->getId() everywhere will ensure that you always get the generated id (the same one everytime) and you are sure you don't get the null initial value.

\$\endgroup\$
0
1
\$\begingroup\$

It seems that the process of generating an ID is going to be the same for all descendants, it will only differ by the actual class used. So how about doing the work outside the class?

abstract class Badge
{
  private string $id;
  protected array $activities;
 
  final public function __construct(string $id, array $activities)
  {
    $this->id = $id;
    $this->activities = $activities;
  }

  public function getId(): string
  {
    return $this->id;
  }

  public function toArray(): array
  {
    return ['id' => $this->id];
  }
}

/**
 * @template T of Badge
 * @param class-string<T> $class
 * @param array $activities
 * @return T
 */
function createBadge(string $class, array $activities, string $target)
{
  $parts = explode('\\', get_class($this));
  $classWithoutNamespace = array_pop($parts);
  $snakeCased = strtolower(preg_replace('/(?<!^)[A-Z]/', '_$0', $classWithoutNamespace));

  return new $class("{$snakeCased}_{$target}", $activities);
}

$badgeX = createBadge(BadgeX::class, $activitiesX, $targetX);
$badgeY = createBadge(BadgeY::class, $activitiesY, $targetY);

Notice that the $target comes from your original code where it is referenced but never defined. I don't know what it is or how to treat it. Depending on the context my review could have pointed out that the abstract class may be a sign of bad design. But unless you provide more context, I cannot say...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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