Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Breach of Contract + enforcing a bad one:

Breach of Contract + enforcing a bad one:

#Update

Update

#Breach of Contract + enforcing a bad one:

#Update

Breach of Contract + enforcing a bad one:

Update

Now these are just the first things I could think of, each of which are problematic enough to ditch the trait as you rightfully did.
Allow me to explain, though, what these one-liners mean:

Allow me to explain, though, what these one-liners mean:

If the base class' constructor, for example, expects 1 argument of the type array (enforced through type-hinting), then all of its children should either use the same constructor or, if they choose to override it, define a constructor that takes 1 arrrayarray argument. They are allowed to loose the type-hint, but may not change it. They're free to add optional arguments, but can't force additional arguments to be passed to the constructor:

Now this was a simple example, but if you want a more plausible scenario: Assume 2 traits, A and B, both define a method logError, A::logError writes to a default log file, B is more generic, and expects you to pass a resource as an argument, or writes to stderr by default.
Because

Because your project is rather large, you're dealing with multiple levels of inheritance and at some point, some classes end up implementing both traits. Of course, depending on what class it is, either A::logError is given a higher precedence, or B:logError. The latter is more likely to be used in workers/crons/backend code.

$traits = class_uses($object);
if (isset($tratis['A']$traits['A']))
{
 $object->logError('The error string');
}

Now if this object has choseschosen to override A::logError with B::logError, you'll probably waste a lot of time trying to work out why on earth logError doesn't write your error messages to the default log. In short:

Back to the definition of abstract methods in a trait: I'll have to test it a bit more, but AFAIK, that's just bad design. Either an abstract class contains your abstract methods (because a class enforces a strong contract), or you implement the method in the class from the get-go. A trait is supposed to reduce code duplication, whereas an abstract method requires you to re-write the method's signature and implementation whenever you use the trait. If that's what you want, why bother using a trait in the first place?
The

The other issue I have with this is one I'll look into a bit more when I find the time, but classes can be composed out of 1 or more traits, and contain no other definitions of their own. That's fine, but what if we look at our 2 traits above (Pure and Evil). Is Pure::setDependency a valid implementation of the abstract Evil::setDependency method? If so, will that trait method be regarded as an implementation of the abstract method? If not, traits containing abstracts can't be used in classes that contain no definitions of their own, and therefore, such traits are special cases (because their usage is limited).

Now these are just the first things I could think of, each of which are problematic enough to ditch the trait as you rightfully did.
Allow me to explain, though, what these one-liners mean:

If the base class' constructor, for example, expects 1 argument of the type array (enforced through type-hinting), then all of its children should either use the same constructor or, if they choose to override it, define a constructor that takes 1 arrray argument. They are allowed to loose the type-hint, but may not change it. They're free to add optional arguments, but can't force additional arguments to be passed to the constructor:

Now this was a simple example, but if you want a more plausible scenario: Assume 2 traits, A and B, both define a method logError, A::logError writes to a default log file, B is more generic, and expects you to pass a resource as an argument, or writes to stderr by default.
Because your project is rather large, you're dealing with multiple levels of inheritance and at some point, some classes end up implementing both traits. Of course, depending on what class it is, either A::logError is given a higher precedence, or B:logError. The latter is more likely to be used in workers/crons/backend code.

$traits = class_uses($object);
if (isset($tratis['A']))
{
 $object->logError('The error string');
}

Now if this object has choses to override A::logError with B::logError, you'll probably waste a lot of time trying to work out why on earth logError doesn't write your error messages to the default log. In short:

Back to the definition of abstract methods in a trait: I'll have to test it a bit more, but AFAIK, that's just bad design. Either an abstract class contains your abstract methods (because a class enforces a strong contract), or you implement the method in the class from the get-go. A trait is supposed to reduce code duplication, whereas an abstract method requires you to re-write the method's signature and implementation whenever you use the trait. If that's what you want, why bother using a trait in the first place?
The other issue I have with this is one I'll look into a bit more when I find the time, but classes can be composed out of 1 or more traits, and contain no other definitions of their own. That's fine, but what if we look at our 2 traits above (Pure and Evil). Is Pure::setDependency a valid implementation of the abstract Evil::setDependency method? If so, will that trait method be regarded as an implementation of the abstract method? If not, traits containing abstracts can't be used in classes that contain no definitions of their own, and therefore, such traits are special cases (because their usage is limited).

Now these are just the first things I could think of, each of which are problematic enough to ditch the trait as you rightfully did.

Allow me to explain, though, what these one-liners mean:

If the base class' constructor, for example, expects 1 argument of the type array (enforced through type-hinting), then all of its children should either use the same constructor or, if they choose to override it, define a constructor that takes 1 array argument. They are allowed to loose the type-hint, but may not change it. They're free to add optional arguments, but can't force additional arguments to be passed to the constructor:

Now this was a simple example, but if you want a more plausible scenario: Assume 2 traits, A and B, both define a method logError, A::logError writes to a default log file, B is more generic, and expects you to pass a resource as an argument, or writes to stderr by default.

Because your project is rather large, you're dealing with multiple levels of inheritance and at some point, some classes end up implementing both traits. Of course, depending on what class it is, either A::logError is given a higher precedence, or B:logError. The latter is more likely to be used in workers/crons/backend code.

$traits = class_uses($object);
if (isset($traits['A']))
{
 $object->logError('The error string');
}

Now if this object has chosen to override A::logError with B::logError, you'll probably waste a lot of time trying to work out why on earth logError doesn't write your error messages to the default log. In short:

Back to the definition of abstract methods in a trait: I'll have to test it a bit more, but AFAIK, that's just bad design. Either an abstract class contains your abstract methods (because a class enforces a strong contract), or you implement the method in the class from the get-go. A trait is supposed to reduce code duplication, whereas an abstract method requires you to re-write the method's signature and implementation whenever you use the trait. If that's what you want, why bother using a trait in the first place?

The other issue I have with this is one I'll look into a bit more when I find the time, but classes can be composed out of 1 or more traits, and contain no other definitions of their own. That's fine, but what if we look at our 2 traits above (Pure and Evil). Is Pure::setDependency a valid implementation of the abstract Evil::setDependency method? If so, will that trait method be regarded as an implementation of the abstract method? If not, traits containing abstracts can't be used in classes that contain no definitions of their own, and therefore, such traits are special cases (because their usage is limited).

added 4811 characters in body
Source Link

#Update

Time for an update. While I stand (firmly) by what I've stated previously (traits not being in a position to impose a contract), there are certainly people who will argue that they can. Not in the least because PHP's trait construct enables you to declare abstract methods:

trait Evil
{
 protected $dependency = null;
 public function someMethod()
 {
 return $this->dependency->getValue();
 }
 abstract public function setDependency(SomeType $dep);
}

This trait, then, imposes a partial contract on its user: the class that uses the trait must define the setDependency method. So what's the problem, you might say? The problem is simple: traits were introduced to solve supposed problems that might require multiple inheritance, hence a class can use more than one trait, and it's possible for the class to resolve conflicts regarding method names by itself. So let's assume the trait above is being used, and you have a couple more traits lying around, one of which looks like this:

trait Pure
{
 public $dependency = null;
 public function setDependency(Container $container)
 {
 $this->dependency = $container;
 return $this;
 }
}

Now, imagine you have a class that uses both these traits (assume Evil::setDependency is not declared abstract here):

class FromHell
{
 use Pure, Evil {
 Pure::setDependency insteadof Evil;
 }
}

Congrats, the contract imposed by the trait is broken, along with its functionality, simply because the class has chosen to adhere to the contract imposed by Pure. It doesn't take a rocket scientist to realize that traits, whilst they can, in theory, impose a minimal contract, are not as potent at ensuring the contract is adhered to.

Now this was a simple example, but if you want a more plausible scenario: Assume 2 traits, A and B, both define a method logError, A::logError writes to a default log file, B is more generic, and expects you to pass a resource as an argument, or writes to stderr by default.
Because your project is rather large, you're dealing with multiple levels of inheritance and at some point, some classes end up implementing both traits. Of course, depending on what class it is, either A::logError is given a higher precedence, or B:logError. The latter is more likely to be used in workers/crons/backend code.

It's very likely that your project, seeing as you're relying on traits so much, contains code that looks like this:

$traits = class_uses($object);
if (isset($tratis['A']))
{
 $object->logError('The error string');
}

Now if this object has choses to override A::logError with B::logError, you'll probably waste a lot of time trying to work out why on earth logError doesn't write your error messages to the default log. In short:

Trait contracts are weak suggestions, and can be overridden easily

Back to the definition of abstract methods in a trait: I'll have to test it a bit more, but AFAIK, that's just bad design. Either an abstract class contains your abstract methods (because a class enforces a strong contract), or you implement the method in the class from the get-go. A trait is supposed to reduce code duplication, whereas an abstract method requires you to re-write the method's signature and implementation whenever you use the trait. If that's what you want, why bother using a trait in the first place?
The other issue I have with this is one I'll look into a bit more when I find the time, but classes can be composed out of 1 or more traits, and contain no other definitions of their own. That's fine, but what if we look at our 2 traits above (Pure and Evil). Is Pure::setDependency a valid implementation of the abstract Evil::setDependency method? If so, will that trait method be regarded as an implementation of the abstract method? If not, traits containing abstracts can't be used in classes that contain no definitions of their own, and therefore, such traits are special cases (because their usage is limited).

If Pure::setDependencyis a valid implementation of the abstract method in Evil (Container being an alias of SomeType, or its parent class), and traits can be used to implement abstract trait methods, then another issue presents itself: maintenance HELL: Changing either one of these traits could break your code, depending on how, and where the traits are being used. You'll end up having to test all classes using both traits, whenever you change just one trait. When you look at it like that, I'd say some code duplication is the lesser of two evils.


#Update

Time for an update. While I stand (firmly) by what I've stated previously (traits not being in a position to impose a contract), there are certainly people who will argue that they can. Not in the least because PHP's trait construct enables you to declare abstract methods:

trait Evil
{
 protected $dependency = null;
 public function someMethod()
 {
 return $this->dependency->getValue();
 }
 abstract public function setDependency(SomeType $dep);
}

This trait, then, imposes a partial contract on its user: the class that uses the trait must define the setDependency method. So what's the problem, you might say? The problem is simple: traits were introduced to solve supposed problems that might require multiple inheritance, hence a class can use more than one trait, and it's possible for the class to resolve conflicts regarding method names by itself. So let's assume the trait above is being used, and you have a couple more traits lying around, one of which looks like this:

trait Pure
{
 public $dependency = null;
 public function setDependency(Container $container)
 {
 $this->dependency = $container;
 return $this;
 }
}

Now, imagine you have a class that uses both these traits (assume Evil::setDependency is not declared abstract here):

class FromHell
{
 use Pure, Evil {
 Pure::setDependency insteadof Evil;
 }
}

Congrats, the contract imposed by the trait is broken, along with its functionality, simply because the class has chosen to adhere to the contract imposed by Pure. It doesn't take a rocket scientist to realize that traits, whilst they can, in theory, impose a minimal contract, are not as potent at ensuring the contract is adhered to.

Now this was a simple example, but if you want a more plausible scenario: Assume 2 traits, A and B, both define a method logError, A::logError writes to a default log file, B is more generic, and expects you to pass a resource as an argument, or writes to stderr by default.
Because your project is rather large, you're dealing with multiple levels of inheritance and at some point, some classes end up implementing both traits. Of course, depending on what class it is, either A::logError is given a higher precedence, or B:logError. The latter is more likely to be used in workers/crons/backend code.

It's very likely that your project, seeing as you're relying on traits so much, contains code that looks like this:

$traits = class_uses($object);
if (isset($tratis['A']))
{
 $object->logError('The error string');
}

Now if this object has choses to override A::logError with B::logError, you'll probably waste a lot of time trying to work out why on earth logError doesn't write your error messages to the default log. In short:

Trait contracts are weak suggestions, and can be overridden easily

Back to the definition of abstract methods in a trait: I'll have to test it a bit more, but AFAIK, that's just bad design. Either an abstract class contains your abstract methods (because a class enforces a strong contract), or you implement the method in the class from the get-go. A trait is supposed to reduce code duplication, whereas an abstract method requires you to re-write the method's signature and implementation whenever you use the trait. If that's what you want, why bother using a trait in the first place?
The other issue I have with this is one I'll look into a bit more when I find the time, but classes can be composed out of 1 or more traits, and contain no other definitions of their own. That's fine, but what if we look at our 2 traits above (Pure and Evil). Is Pure::setDependency a valid implementation of the abstract Evil::setDependency method? If so, will that trait method be regarded as an implementation of the abstract method? If not, traits containing abstracts can't be used in classes that contain no definitions of their own, and therefore, such traits are special cases (because their usage is limited).

If Pure::setDependencyis a valid implementation of the abstract method in Evil (Container being an alias of SomeType, or its parent class), and traits can be used to implement abstract trait methods, then another issue presents itself: maintenance HELL: Changing either one of these traits could break your code, depending on how, and where the traits are being used. You'll end up having to test all classes using both traits, whenever you change just one trait. When you look at it like that, I'd say some code duplication is the lesser of two evils.

Source Link
Loading
lang-php

AltStyle によって変換されたページ (->オリジナル) /