Insphpect

This tool is currently proof-of-concept. Your feedback and evaluation is valuable in helping to improve it and ensure its reports are meaninful.

Please click here to complete a short survey to tell us what you think. It should take less than 5 minutes and help further this research project!

GuzzleHttp\Cookie\CookieJar

Detected issues

Issue Method Line number
Using `new` in constructor __construct 31
Use of static methods self 43
Use of static methods bool 65
Annotations N/A 94
Annotations N/A 104
Annotations N/A 139
Annotations N/A 152

Code

Click highlighted lines for details

<?phpnamespace GuzzleHttp\Cookie;use Psr\Http\Message\RequestInterface;use Psr\Http\Message\ResponseInterface;/** * Cookie jar that stores cookies as an array */class CookieJar implements CookieJarInterface{    /** @var SetCookie[] Loaded cookie data */    private $cookies = [];    /** @var bool */    private $strictMode;    /**     * @param bool  $strictMode  Set to true to throw exceptions when invalid     *                           cookies are added to the cookie jar.     * @param array $cookieArray Array of SetCookie objects or a hash of     *                           arrays that can be used with the SetCookie     *                           constructor     */    public function __construct(bool $strictMode = false, array $cookieArray = [])    {        $this->strictMode = $strictMode;        foreach ($cookieArray as $cookie) {            if (!($cookie instanceof SetCookie)) {

Usage of the new keyword in a constructor

Summary of issues

Background

If a dependency is constructed inside the object that requires it rather than passed in as a reference then flexibility is lost[1][2]

 public class Car {

    private 
Engine engine;
    
    public 
Car() {
        
this.engine = new Engine();
    }
}

Here, the Car constructor creates the Engine instance. This is inflexible as it forces all Car objects to use the exact same Engine type. Instead, it would encourage reuse if the program supported different engine types (e.g. DieselEngine, PetrolEngine or HybridEngine).

The same is true when an instance variable is created when the class is defined:

 public class Car {
    private 
Engine engine = new Engine();
}

By using the new keyword to instantiate a dependency, the specific implementation of that dependency is hardcoded and cannot be substituted.

Instead, the dependency should be constructed outside the class and injected in:

 public class Car {
    private 
Engine engine;

    public 
Car(Engine engine) {
        
this.engine engine;
    }
}

Using dependency injection it is possible to pass in any engine type:

 //Instead of
Car myCar = new Car();

//It's now possible to construct different types of car:
Car petrolCar = new Car(new PetrolEngine);
Car electricCar = new Car(new ElectricEngine);

A secondary advantage to Dependency Injection with regards to flexibility and encapsulation is that the class which has the dependency (Car, in this example) it not aware of the dependencies of the Engine class.

For example, if the Engine class required a Gearbox instance as a constructor argument, the Car class would need to instantiate and pass in the relevant Gearbox instance. And provide any dependencies of the Gearbox class when instantiating it.

If the constructor arguments of any of the classes which need to be instantiated are modified during development, any class which creates an instance of the class must also be modified. A change to the constructor for Engine would require modifying the Car class. Instead, if the fully constructed Engine instance

By loosely coupling the Engine class to the Car class, the author of the Car class does not need to know anything about the implementation of Engine class or have knowledge of what dependencies it has.

 
public Car() {

    
this.engine = new Engine(new Gearbox());
}

Further reading

Additional resources:

References

  1. Hevery, M. (2008) How to Think About the “new” Operator with Respect to Unit Testing [online]. Available from: http://misko.hevery.com/2008/07/08/how-to-think-about-the-new-operator/
  2. Hevery, M. (2008) Code Reviewers Guide [online]. Available from: http://misko.hevery.com/code-reviewers-guide/

Please note: This feature is currently proof-of-concept, the instructions may not always be completely accurate.

\GuzzleHttp\Cookie\SetCookie is instantiated inside the constructor of GuzzleHttp\Cookie\CookieJar

1) Remove the new expression and replace it with a variable:

 

$cookie = new SetCookie($cookie);

becomes:

 

                $cookie = new SetCookie($cookie);

2) Add a constructor argument for the new variable: Replace:

 

public function __construct(bool $strictMode false, array $cookieArray = [])

with:

 

public function __construct(bool $strictMode false, array $cookieArray = [])

3) Find any occurance of new GuzzleHttp\Cookie\CookieJar and provide the new dependency.

\GuzzleHttp\Cookie\CookieJar is being instantiated in src/Client.php:0

Replace:

 

$this->config['cookies'] = new CookieJar();

With:

Please note: This feature is currently proof-of-concept, this patch may not work, please don't blindly apply it.

diff --git a/src/Client.php b/src/Client.php
index 4c90bd0..0a6ac6e 100644
--- a/src/Client.php
+++ b/src/Client.php
@@ -250,7 +250,7 @@ class Client implements ClientInterface, \Psr\Http\Client\ClientInterface
         $this->config = $config + $defaults;

         if (!empty($config['cookies']) && $config['cookies'] === true) {
-            $this->config['cookies'] = new CookieJar();
+            $this->config['cookies'] = new CookieJar(new \GuzzleHttp\Cookie\SetCookie(new SetCookie()));
         }

         // Add the default user-agent header.
diff --git a/src/Cookie/CookieJar.php b/src/Cookie/CookieJar.php
index 341e608..8852e3d 100644
--- a/src/Cookie/CookieJar.php
+++ b/src/Cookie/CookieJar.php
@@ -22,13 +22,13 @@ class CookieJar implements CookieJarInterface
      *                           arrays that can be used with the SetCookie
      *                           constructor
      */
-    public function __construct(bool $strictMode = false, array $cookieArray = [])
+    public function __construct(\GuzzleHttp\Cookie\SetCookie $setCookie, bool $strictMode = false, array $cookieArray = [])
     {
         $this->strictMode = $strictMode;

         foreach ($cookieArray as $cookie) {
             if (!($cookie instanceof SetCookie)) {
-                $cookie = new SetCookie($cookie);
+                $cookie = $setCookie;
             }
             $this->setCookie($cookie);
         }
@@ -302,4 +302,4 @@ class CookieJar implements CookieJarInterface
             );
         }
     }
-}
+}
\ No newline at end of file
diff --git a/src/Pool.php b/src/Pool.php
index ea55eff..0bbb495 100644
--- a/src/Pool.php
+++ b/src/Pool.php
@@ -32,7 +32,7 @@ class Pool implements PromisorInterface
      *                                  - fulfilled: (callable) Function to invoke when a request completes.
      *                                  - rejected: (callable) Function to invoke when a request is rejected.
      */
-    public function __construct(
+    public function __construct(\GuzzleHttp\Promise\EachPromise $eachPromise,
         ClientInterface $client,
         $requests,
         array $config = []
@@ -64,7 +64,7 @@ class Pool implements PromisorInterface
             }
         };

-        $this->each = new EachPromise($requests(), $config);
+        $this->each = $eachPromise, $config);
     }

     /**
@@ -125,4 +125,4 @@ class Pool implements PromisorInterface
             };
         }
     }
-}
+}
\ No newline at end of file
} $this->setCookie($cookie); } } /** * Create a new Cookie jar from an associative array and domain. * * @param array $cookies Cookies to create the jar from * @param string $domain Domain to set the cookies to */

Static methods

Summary of issues

  • Tight Coupling
  • Hidden dependencies
  • Global state (if also using static variables)

Tight Coupling

Use of static methods always reduces flexibility by introducing tight coupling[1]. A static method tightly couples the calling code to the specific class the method exists in.

 
function totalAbs(double valuedouble value2) {
    return 
abs(value) + abs(value2);
}

Here, the method totalAbs has a dependency on the Math class and the .abs() method will always be called. Although for testing purposes this may not be a problem, the coupling reduces flexibility because the total method can only work with doubles/integers, as that's all the Math.abs() function can use. Although type coercion will allow the use of any primitive numeric type, these types have limitations. It's impossible to use another class such as BigInteger or a class for dealing with greater precision decimals or even alternative numbering systems such as Roman numerals.

The totalAbs function takes two doubles and converts them to their absolute values before adding them. This is inflexible because it only works with doubles. It's tied to doubles because that's what the Math.abs() static method requires. If, instead, using OOP an interface was created to handle any number that had this method:

 interface Numeric {
    public function 
abs(): Numeric;
}

It would then be possible to rewrite the totalAbs method to work with any kind of number:

 function totalAbs(Numeric valueNumeric value): Numeric {
    return 
value.abs() + value2.abs();
}

By removing the static method and using an instance method in its place the totalAbs method is now agnostic about the type of number it is dealing with. It could be called with any of the following (assuming they implement the Numeric interface)

 
totalAbs(new Integer(4), new Integer(-53));

totalAbs(new Double(34.4), new Integer(-2));

totalAbs(new BigInteger('123445454564765739878989343225778'), new Integer(2343));

totalAbs(new RomanNumeral('VII'), new RomanNumeral('CXV'));

Making the method reusable in a way that it wasn't when static methods were being used. By changing the static methods to instance methods, flexibility has been enhanced as the method can be used with any numeric type, not just numeric types that are supported by the Math.abs() method.

Broken encapsulation

Static methods also break encapsulation. Encapsulation is defined by Rogers[2] as:

the bundling of data with the methods that operate on that data

By passing the numeric value into the abs method, the data being operated on is being separated from the methods that operate on it, breaking encapsulation. Instead using num.abs() the data is encapsulated in the num instance and its type is not visible or relevant to the outside world. abs() will work on the data and work regardless of num's type, providing it implements the abs method.

This is a simple example, but applies to all static methods. Use of polymorphic instance methods that work on encapsulated data will always be more flexible than static method calls which can only ever deal with specific pre-defined types.

Further reading

Exceptions

The only exception to this rule is when a static method is used for object creation in place of the new keyword[3]. This is because the new keyword is already a static call. However, even here a non-static factory is often preferable for testing purposes[4][5].

References

  1. Popov, N. (2014) Don't be STUPID: GRASP SOLID! [online]. Available from: https://nikic.github.io/2011/12/27/Dont-be-STUPID-GRASP-SOLID.html
  2. Rogers, P. (2001) Encapsulation is not information hiding [online]. Available from: http://www.javaworld.com/article/2075271/core-java/encapsulation-is-not-information-hiding.html
  3. Sonmez, J. (2010) Static Methods Will Shock You [online]. Available from: http://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
  4. Hevery, M. (2008) Static Methods are Death to Testability [online]. Available from: http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/
  5. Butler, T. (2013) Are Static Methods/Variables bad practice? [online]. Available from: https://r.je/static-methods-bad-practice.html
{ $cookieJar = new self(); foreach ($cookies as $name => $value) { $cookieJar->setCookie(new SetCookie([ 'Domain' => $domain, 'Name' => $name, 'Value' => $value, 'Discard' => true ])); } return $cookieJar; } /** * Evaluate if this cookie should be persisted to storage * that survives between requests. * * @param SetCookie $cookie Being evaluated. * @param bool $allowSessionCookies If we should persist session cookies */

Static methods

Summary of issues

  • Tight Coupling
  • Hidden dependencies
  • Global state (if also using static variables)

Tight Coupling

Use of static methods always reduces flexibility by introducing tight coupling[1]. A static method tightly couples the calling code to the specific class the method exists in.

 
function totalAbs(double valuedouble value2) {
    return 
abs(value) + abs(value2);
}

Here, the method totalAbs has a dependency on the Math class and the .abs() method will always be called. Although for testing purposes this may not be a problem, the coupling reduces flexibility because the total method can only work with doubles/integers, as that's all the Math.abs() function can use. Although type coercion will allow the use of any primitive numeric type, these types have limitations. It's impossible to use another class such as BigInteger or a class for dealing with greater precision decimals or even alternative numbering systems such as Roman numerals.

The totalAbs function takes two doubles and converts them to their absolute values before adding them. This is inflexible because it only works with doubles. It's tied to doubles because that's what the Math.abs() static method requires. If, instead, using OOP an interface was created to handle any number that had this method:

 interface Numeric {
    public function 
abs(): Numeric;
}

It would then be possible to rewrite the totalAbs method to work with any kind of number:

 function totalAbs(Numeric valueNumeric value): Numeric {
    return 
value.abs() + value2.abs();
}

By removing the static method and using an instance method in its place the totalAbs method is now agnostic about the type of number it is dealing with. It could be called with any of the following (assuming they implement the Numeric interface)

 
totalAbs(new Integer(4), new Integer(-53));

totalAbs(new Double(34.4), new Integer(-2));

totalAbs(new BigInteger('123445454564765739878989343225778'), new Integer(2343));

totalAbs(new RomanNumeral('VII'), new RomanNumeral('CXV'));

Making the method reusable in a way that it wasn't when static methods were being used. By changing the static methods to instance methods, flexibility has been enhanced as the method can be used with any numeric type, not just numeric types that are supported by the Math.abs() method.

Broken encapsulation

Static methods also break encapsulation. Encapsulation is defined by Rogers[2] as:

the bundling of data with the methods that operate on that data

By passing the numeric value into the abs method, the data being operated on is being separated from the methods that operate on it, breaking encapsulation. Instead using num.abs() the data is encapsulated in the num instance and its type is not visible or relevant to the outside world. abs() will work on the data and work regardless of num's type, providing it implements the abs method.

This is a simple example, but applies to all static methods. Use of polymorphic instance methods that work on encapsulated data will always be more flexible than static method calls which can only ever deal with specific pre-defined types.

Further reading

Exceptions

The only exception to this rule is when a static method is used for object creation in place of the new keyword[3]. This is because the new keyword is already a static call. However, even here a non-static factory is often preferable for testing purposes[4][5].

References

  1. Popov, N. (2014) Don't be STUPID: GRASP SOLID! [online]. Available from: https://nikic.github.io/2011/12/27/Dont-be-STUPID-GRASP-SOLID.html
  2. Rogers, P. (2001) Encapsulation is not information hiding [online]. Available from: http://www.javaworld.com/article/2075271/core-java/encapsulation-is-not-information-hiding.html
  3. Sonmez, J. (2010) Static Methods Will Shock You [online]. Available from: http://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
  4. Hevery, M. (2008) Static Methods are Death to Testability [online]. Available from: http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/
  5. Butler, T. (2013) Are Static Methods/Variables bad practice? [online]. Available from: https://r.je/static-methods-bad-practice.html
{ if ($cookie->getExpires() || $allowSessionCookies) { if (!$cookie->getDiscard()) { return true; } } return false; } /** * Finds and returns the cookie based on the name * * @param string $name cookie name to search for * * @return SetCookie|null cookie that was found or null if not found */ public function getCookieByName(string $name): ?SetCookie { foreach ($this->cookies as $cookie) { if ($cookie->getName() !== null && \strcasecmp($cookie->getName(), $name) === 0) { return $cookie; } } return null; } /**

Annotations are widely adopted in Java[1] and are slowly being adopted into other languages [2], however, in most cases they break the fundamental Object-Oriented principal of encapsulation and limit flexibility in the process.

When used for configuration as opposed to documentation annotations cause a large set of problems. For example, a common use-case for annotations is @Inject to tell an external Dependency Injection Container to set a specific private property to the given dependency[3]:

 public class Product {
  @
Inject
  private Database db;
}

Here, @Inject signals to the Dependency Injection Container that a Database instance must be written to the private property. In a lot of common usage, no constructor is even supplied [4]. This means it's impossible to use the class without a dependency injection container.

The author of the class has written the code in the expectation that it will be created by a Dependency Injection Container. This breaks encapsulation as there is implied knowledge of other parts of the application within this class, knowledge that it will be used in an environment where a Dependency Injection Container looks for the annotation and supplies the dependency. This severely limits flexibility because there is no easy way to construct the class without using a Dependency Injection Container that understands @Inject and knows to look for it.

Encapsulation is broken because the class is no longer in control of its own state, it assumes that it will be running in a very specific environment.

If the class is moved to a project using a different container, or even no dependency injection container, it's usefulness is severely limited because there is no way to set the database dependency.

Instead, the code above should be written as:

 public class Product {
  private 
Database db;

  
Product(Database db) {
      
this.db db;
  }
}

Here, flexibility has been greatly enhanced because the class has no knowledge of the environment is is being used in. There may be a dependency injection container, there may not.

Encapsulation is now maintained, there is no way to instantiate the class without the constructor being run and dependencies supplied.

By using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the Single responsibility principleBy using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the [Single responsibility principle](#srp) and Encapsulation.

Allthough Inject is a common annotation, the problems exist anywhere annotations are used for application configuration.

They store metadata about the class and how the class should be used. This becomes a problem when different projects need different configurations for the class, for example annotations are commonly used for URL routing [5][6]:

 @Path("/users/{username}")
public class 
UserResource {

    @
GET
    @Produces("text/xml")
    public 
String getUser(@PathParam("username"String userName) {
        ...
    }
}

This is used to tell a web-server the URI the class will handle. In the example above, the annotation sets the route to users. This tight coupling of the metadata to the class causes flexibility issues. It's not unreasonable to want to use a class that deals with users on more than one website. However, because the class uses internal metadata using annotations, it's impossible to use the class on the URI /users on one website and /members/ on another without changing the class. This is a violation of the Single Responsibility Principle as the class has more than one reason to change.

If the class is changed then significant issues are introduced with version control: Once a bug is fixed on one website, it's then difficult to copy it over to the other website as the sites are using independent branches.

The solution is to separate out the metadata from the class using any format, for example JSON to map the URI path to the class name

 {
    
"/users""UserResource",
    
"/Products""Products"
}

Using external metadata, the metadata can differ per website and the class can remain identical, allowing for easier bugfixes and sharing of resources between projects.

As Ahuja[7] writes:

My advice is not to use annotations to configure your applications. Configurations were never meant to be part of code—that's why they're configurations and not source code. So let configurations stay in the configuration files. A small productivity gain in the short term won't go amount to much when a client asks for a change in a table or a value and you tell them it's going to take five days of development, testing and deployment.

There is an ongoing debate among developers about if and when annotations should be used. However, the sole benefit of annotations is being able to edit metadata and code in the same file, the argument in favour of their use is always at the expense of flexibility. This is an debate of convenience vs flexibility. From a flexibility point of view, annotations are considerably worse than available alternatives.

It should also be noted that this only applies to annotations which adjust the program's outcome. If removing the annotations does not affect how the program works, the annotations are there for documentation and do not cause any problems with flexibility.

As noted by Walls[8], annotations also break the law of demeter by adding extra dependencies to classes:

But, as luck would have it, SearchController also transitively depends on @Searchable and any other Compass annotations I use in Document. That means that I can't compile the web module without it having Compass in its build classpath (that is, as a dependency in its POM. Even though SearchController doesn't know about or need to know about Compass!
Doesn't this violate the law of demeter?

Although this problem is specific to Java, in other languages annotations will not introduce a hard dependency.

However, there is a remaining issue of comprehension. You could move a class with @Inject or similar annotation to a project where the annotations are just comments. Anyone looking at the class in this project will assume that the annotations are used and will be surprised when they change the annotations and it doesn't affect the configuration. This is not directly an issue of flexibility but it breaks the Principle of Least Surprise[9][10] and makes the class more difficult to reuse because it's not clear to anyone reading the code.

Summary of problems:

  • Cannot be debugged easily as you can't print the contents of the configuration
  • Finding application configuration requires looking across every class in the application
  • Breaks the Single Responsibility Principle
  • Breaks Encapsulation
  • Breaks Separation of Concerns
  • Introduces ambiguity/bugs in polymorphic code
  • Introduces coupling between unrelated components
  • Makes it more difficult to instantiate an object with different configurations
  • Makes version control more difficult

References

  1. Reigler, G. (2014) An Annotation Nightmare [online]. Available from: http://www.javacodegeeks.com/2014/01/an-annotation-nightmare.html
  2. Dohms, R. (2013) Annotations in PHP: They Exist [online]. Available from: http://www.slideshare.net/rdohms/annotations-in-php-they-exist
  3. Oracle, O. (2011) Annotation Type Inject [online]. Available from: http://docs.oracle.com/javaee/6/api/javax/inject/Inject.html
  4. Bugayenko, Y. (2016) Java Annotations Are a Big Mistake [online]. Available from: http://www.yegor256.com/2016/04/12/java-annotations-are-evil.html
  5. Oracle, O. (2010) The @Path Annotation and URI Path Templates [online]. Available from: https://docs.oracle.com/cd/E19798-01/821-1841/ginpw/
  6. Symfony Framework, S. (nd) @Route and @Method [online]. Available from: http://symfony.com/doc/2.0/bundles/SensioFrameworkExtraBundle/annotations/routing.html
  7. Ahuja, K. (2015) Are Annotations Bad? [online]. Available from: https://dzone.com/articles/are-annotations-bad
  8. Walls, C. (2008) When Good Annotations Go Bad [online]. Available from: http://java.dzone.com/articles/when-good-annotations-go-bad
  9. Raymond, E. (2003) The Art of Unix Programming ISBN: 978-0131429017. Addison Wesley.
  10. James, G. (1987) The Tao of Programming ISBN: 978-0931137075. Info Books.
*/ public function toArray(): array { return \array_map(function (SetCookie $cookie): array { return $cookie->toArray(); }, $this->getIterator()->getArrayCopy()); } /**

Annotations are widely adopted in Java[1] and are slowly being adopted into other languages [2], however, in most cases they break the fundamental Object-Oriented principal of encapsulation and limit flexibility in the process.

When used for configuration as opposed to documentation annotations cause a large set of problems. For example, a common use-case for annotations is @Inject to tell an external Dependency Injection Container to set a specific private property to the given dependency[3]:

 public class Product {
  @
Inject
  private Database db;
}

Here, @Inject signals to the Dependency Injection Container that a Database instance must be written to the private property. In a lot of common usage, no constructor is even supplied [4]. This means it's impossible to use the class without a dependency injection container.

The author of the class has written the code in the expectation that it will be created by a Dependency Injection Container. This breaks encapsulation as there is implied knowledge of other parts of the application within this class, knowledge that it will be used in an environment where a Dependency Injection Container looks for the annotation and supplies the dependency. This severely limits flexibility because there is no easy way to construct the class without using a Dependency Injection Container that understands @Inject and knows to look for it.

Encapsulation is broken because the class is no longer in control of its own state, it assumes that it will be running in a very specific environment.

If the class is moved to a project using a different container, or even no dependency injection container, it's usefulness is severely limited because there is no way to set the database dependency.

Instead, the code above should be written as:

 public class Product {
  private 
Database db;

  
Product(Database db) {
      
this.db db;
  }
}

Here, flexibility has been greatly enhanced because the class has no knowledge of the environment is is being used in. There may be a dependency injection container, there may not.

Encapsulation is now maintained, there is no way to instantiate the class without the constructor being run and dependencies supplied.

By using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the Single responsibility principleBy using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the [Single responsibility principle](#srp) and Encapsulation.

Allthough Inject is a common annotation, the problems exist anywhere annotations are used for application configuration.

They store metadata about the class and how the class should be used. This becomes a problem when different projects need different configurations for the class, for example annotations are commonly used for URL routing [5][6]:

 @Path("/users/{username}")
public class 
UserResource {

    @
GET
    @Produces("text/xml")
    public 
String getUser(@PathParam("username"String userName) {
        ...
    }
}

This is used to tell a web-server the URI the class will handle. In the example above, the annotation sets the route to users. This tight coupling of the metadata to the class causes flexibility issues. It's not unreasonable to want to use a class that deals with users on more than one website. However, because the class uses internal metadata using annotations, it's impossible to use the class on the URI /users on one website and /members/ on another without changing the class. This is a violation of the Single Responsibility Principle as the class has more than one reason to change.

If the class is changed then significant issues are introduced with version control: Once a bug is fixed on one website, it's then difficult to copy it over to the other website as the sites are using independent branches.

The solution is to separate out the metadata from the class using any format, for example JSON to map the URI path to the class name

 {
    
"/users""UserResource",
    
"/Products""Products"
}

Using external metadata, the metadata can differ per website and the class can remain identical, allowing for easier bugfixes and sharing of resources between projects.

As Ahuja[7] writes:

My advice is not to use annotations to configure your applications. Configurations were never meant to be part of code—that's why they're configurations and not source code. So let configurations stay in the configuration files. A small productivity gain in the short term won't go amount to much when a client asks for a change in a table or a value and you tell them it's going to take five days of development, testing and deployment.

There is an ongoing debate among developers about if and when annotations should be used. However, the sole benefit of annotations is being able to edit metadata and code in the same file, the argument in favour of their use is always at the expense of flexibility. This is an debate of convenience vs flexibility. From a flexibility point of view, annotations are considerably worse than available alternatives.

It should also be noted that this only applies to annotations which adjust the program's outcome. If removing the annotations does not affect how the program works, the annotations are there for documentation and do not cause any problems with flexibility.

As noted by Walls[8], annotations also break the law of demeter by adding extra dependencies to classes:

But, as luck would have it, SearchController also transitively depends on @Searchable and any other Compass annotations I use in Document. That means that I can't compile the web module without it having Compass in its build classpath (that is, as a dependency in its POM. Even though SearchController doesn't know about or need to know about Compass!
Doesn't this violate the law of demeter?

Although this problem is specific to Java, in other languages annotations will not introduce a hard dependency.

However, there is a remaining issue of comprehension. You could move a class with @Inject or similar annotation to a project where the annotations are just comments. Anyone looking at the class in this project will assume that the annotations are used and will be surprised when they change the annotations and it doesn't affect the configuration. This is not directly an issue of flexibility but it breaks the Principle of Least Surprise[9][10] and makes the class more difficult to reuse because it's not clear to anyone reading the code.

Summary of problems:

  • Cannot be debugged easily as you can't print the contents of the configuration
  • Finding application configuration requires looking across every class in the application
  • Breaks the Single Responsibility Principle
  • Breaks Encapsulation
  • Breaks Separation of Concerns
  • Introduces ambiguity/bugs in polymorphic code
  • Introduces coupling between unrelated components
  • Makes it more difficult to instantiate an object with different configurations
  • Makes version control more difficult

References

  1. Reigler, G. (2014) An Annotation Nightmare [online]. Available from: http://www.javacodegeeks.com/2014/01/an-annotation-nightmare.html
  2. Dohms, R. (2013) Annotations in PHP: They Exist [online]. Available from: http://www.slideshare.net/rdohms/annotations-in-php-they-exist
  3. Oracle, O. (2011) Annotation Type Inject [online]. Available from: http://docs.oracle.com/javaee/6/api/javax/inject/Inject.html
  4. Bugayenko, Y. (2016) Java Annotations Are a Big Mistake [online]. Available from: http://www.yegor256.com/2016/04/12/java-annotations-are-evil.html
  5. Oracle, O. (2010) The @Path Annotation and URI Path Templates [online]. Available from: https://docs.oracle.com/cd/E19798-01/821-1841/ginpw/
  6. Symfony Framework, S. (nd) @Route and @Method [online]. Available from: http://symfony.com/doc/2.0/bundles/SensioFrameworkExtraBundle/annotations/routing.html
  7. Ahuja, K. (2015) Are Annotations Bad? [online]. Available from: https://dzone.com/articles/are-annotations-bad
  8. Walls, C. (2008) When Good Annotations Go Bad [online]. Available from: http://java.dzone.com/articles/when-good-annotations-go-bad
  9. Raymond, E. (2003) The Art of Unix Programming ISBN: 978-0131429017. Addison Wesley.
  10. James, G. (1987) The Tao of Programming ISBN: 978-0931137075. Info Books.
*/ public function clear(?string $domain = null, ?string $path = null, ?string $name = null): void { if (!$domain) { $this->cookies = []; return; } elseif (!$path) { $this->cookies = \array_filter( $this->cookies, function (SetCookie $cookie) use ($domain): bool { return !$cookie->matchesDomain($domain); } ); } elseif (!$name) { $this->cookies = \array_filter( $this->cookies, function (SetCookie $cookie) use ($path, $domain): bool { return !($cookie->matchesPath($path) && $cookie->matchesDomain($domain)); } ); } else { $this->cookies = \array_filter( $this->cookies, function (SetCookie $cookie) use ($path, $domain, $name) { return !($cookie->getName() == $name && $cookie->matchesPath($path) && $cookie->matchesDomain($domain)); } ); } } /**

Annotations are widely adopted in Java[1] and are slowly being adopted into other languages [2], however, in most cases they break the fundamental Object-Oriented principal of encapsulation and limit flexibility in the process.

When used for configuration as opposed to documentation annotations cause a large set of problems. For example, a common use-case for annotations is @Inject to tell an external Dependency Injection Container to set a specific private property to the given dependency[3]:

 public class Product {
  @
Inject
  private Database db;
}

Here, @Inject signals to the Dependency Injection Container that a Database instance must be written to the private property. In a lot of common usage, no constructor is even supplied [4]. This means it's impossible to use the class without a dependency injection container.

The author of the class has written the code in the expectation that it will be created by a Dependency Injection Container. This breaks encapsulation as there is implied knowledge of other parts of the application within this class, knowledge that it will be used in an environment where a Dependency Injection Container looks for the annotation and supplies the dependency. This severely limits flexibility because there is no easy way to construct the class without using a Dependency Injection Container that understands @Inject and knows to look for it.

Encapsulation is broken because the class is no longer in control of its own state, it assumes that it will be running in a very specific environment.

If the class is moved to a project using a different container, or even no dependency injection container, it's usefulness is severely limited because there is no way to set the database dependency.

Instead, the code above should be written as:

 public class Product {
  private 
Database db;

  
Product(Database db) {
      
this.db db;
  }
}

Here, flexibility has been greatly enhanced because the class has no knowledge of the environment is is being used in. There may be a dependency injection container, there may not.

Encapsulation is now maintained, there is no way to instantiate the class without the constructor being run and dependencies supplied.

By using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the Single responsibility principleBy using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the [Single responsibility principle](#srp) and Encapsulation.

Allthough Inject is a common annotation, the problems exist anywhere annotations are used for application configuration.

They store metadata about the class and how the class should be used. This becomes a problem when different projects need different configurations for the class, for example annotations are commonly used for URL routing [5][6]:

 @Path("/users/{username}")
public class 
UserResource {

    @
GET
    @Produces("text/xml")
    public 
String getUser(@PathParam("username"String userName) {
        ...
    }
}

This is used to tell a web-server the URI the class will handle. In the example above, the annotation sets the route to users. This tight coupling of the metadata to the class causes flexibility issues. It's not unreasonable to want to use a class that deals with users on more than one website. However, because the class uses internal metadata using annotations, it's impossible to use the class on the URI /users on one website and /members/ on another without changing the class. This is a violation of the Single Responsibility Principle as the class has more than one reason to change.

If the class is changed then significant issues are introduced with version control: Once a bug is fixed on one website, it's then difficult to copy it over to the other website as the sites are using independent branches.

The solution is to separate out the metadata from the class using any format, for example JSON to map the URI path to the class name

 {
    
"/users""UserResource",
    
"/Products""Products"
}

Using external metadata, the metadata can differ per website and the class can remain identical, allowing for easier bugfixes and sharing of resources between projects.

As Ahuja[7] writes:

My advice is not to use annotations to configure your applications. Configurations were never meant to be part of code—that's why they're configurations and not source code. So let configurations stay in the configuration files. A small productivity gain in the short term won't go amount to much when a client asks for a change in a table or a value and you tell them it's going to take five days of development, testing and deployment.

There is an ongoing debate among developers about if and when annotations should be used. However, the sole benefit of annotations is being able to edit metadata and code in the same file, the argument in favour of their use is always at the expense of flexibility. This is an debate of convenience vs flexibility. From a flexibility point of view, annotations are considerably worse than available alternatives.

It should also be noted that this only applies to annotations which adjust the program's outcome. If removing the annotations does not affect how the program works, the annotations are there for documentation and do not cause any problems with flexibility.

As noted by Walls[8], annotations also break the law of demeter by adding extra dependencies to classes:

But, as luck would have it, SearchController also transitively depends on @Searchable and any other Compass annotations I use in Document. That means that I can't compile the web module without it having Compass in its build classpath (that is, as a dependency in its POM. Even though SearchController doesn't know about or need to know about Compass!
Doesn't this violate the law of demeter?

Although this problem is specific to Java, in other languages annotations will not introduce a hard dependency.

However, there is a remaining issue of comprehension. You could move a class with @Inject or similar annotation to a project where the annotations are just comments. Anyone looking at the class in this project will assume that the annotations are used and will be surprised when they change the annotations and it doesn't affect the configuration. This is not directly an issue of flexibility but it breaks the Principle of Least Surprise[9][10] and makes the class more difficult to reuse because it's not clear to anyone reading the code.

Summary of problems:

  • Cannot be debugged easily as you can't print the contents of the configuration
  • Finding application configuration requires looking across every class in the application
  • Breaks the Single Responsibility Principle
  • Breaks Encapsulation
  • Breaks Separation of Concerns
  • Introduces ambiguity/bugs in polymorphic code
  • Introduces coupling between unrelated components
  • Makes it more difficult to instantiate an object with different configurations
  • Makes version control more difficult

References

  1. Reigler, G. (2014) An Annotation Nightmare [online]. Available from: http://www.javacodegeeks.com/2014/01/an-annotation-nightmare.html
  2. Dohms, R. (2013) Annotations in PHP: They Exist [online]. Available from: http://www.slideshare.net/rdohms/annotations-in-php-they-exist
  3. Oracle, O. (2011) Annotation Type Inject [online]. Available from: http://docs.oracle.com/javaee/6/api/javax/inject/Inject.html
  4. Bugayenko, Y. (2016) Java Annotations Are a Big Mistake [online]. Available from: http://www.yegor256.com/2016/04/12/java-annotations-are-evil.html
  5. Oracle, O. (2010) The @Path Annotation and URI Path Templates [online]. Available from: https://docs.oracle.com/cd/E19798-01/821-1841/ginpw/
  6. Symfony Framework, S. (nd) @Route and @Method [online]. Available from: http://symfony.com/doc/2.0/bundles/SensioFrameworkExtraBundle/annotations/routing.html
  7. Ahuja, K. (2015) Are Annotations Bad? [online]. Available from: https://dzone.com/articles/are-annotations-bad
  8. Walls, C. (2008) When Good Annotations Go Bad [online]. Available from: http://java.dzone.com/articles/when-good-annotations-go-bad
  9. Raymond, E. (2003) The Art of Unix Programming ISBN: 978-0131429017. Addison Wesley.
  10. James, G. (1987) The Tao of Programming ISBN: 978-0931137075. Info Books.
*/ public function clearSessionCookies(): void { $this->cookies = \array_filter( $this->cookies, function (SetCookie $cookie): bool { return !$cookie->getDiscard() && $cookie->getExpires(); } ); } /**

Annotations are widely adopted in Java[1] and are slowly being adopted into other languages [2], however, in most cases they break the fundamental Object-Oriented principal of encapsulation and limit flexibility in the process.

When used for configuration as opposed to documentation annotations cause a large set of problems. For example, a common use-case for annotations is @Inject to tell an external Dependency Injection Container to set a specific private property to the given dependency[3]:

 public class Product {
  @
Inject
  private Database db;
}

Here, @Inject signals to the Dependency Injection Container that a Database instance must be written to the private property. In a lot of common usage, no constructor is even supplied [4]. This means it's impossible to use the class without a dependency injection container.

The author of the class has written the code in the expectation that it will be created by a Dependency Injection Container. This breaks encapsulation as there is implied knowledge of other parts of the application within this class, knowledge that it will be used in an environment where a Dependency Injection Container looks for the annotation and supplies the dependency. This severely limits flexibility because there is no easy way to construct the class without using a Dependency Injection Container that understands @Inject and knows to look for it.

Encapsulation is broken because the class is no longer in control of its own state, it assumes that it will be running in a very specific environment.

If the class is moved to a project using a different container, or even no dependency injection container, it's usefulness is severely limited because there is no way to set the database dependency.

Instead, the code above should be written as:

 public class Product {
  private 
Database db;

  
Product(Database db) {
      
this.db db;
  }
}

Here, flexibility has been greatly enhanced because the class has no knowledge of the environment is is being used in. There may be a dependency injection container, there may not.

Encapsulation is now maintained, there is no way to instantiate the class without the constructor being run and dependencies supplied.

By using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the Single responsibility principleBy using annotations, a dependency is needlessly introduced on the component which reads the annotations and the author of the class has indirect control over how external components can use the class. The class now has two responsibilities: exposing its own interface and telling the outside rules how it should be used. In the example above it is telling the container which dependencies it needs. These are two different responsibilities and this breaks the [Single responsibility principle](#srp) and Encapsulation.

Allthough Inject is a common annotation, the problems exist anywhere annotations are used for application configuration.

They store metadata about the class and how the class should be used. This becomes a problem when different projects need different configurations for the class, for example annotations are commonly used for URL routing [5][6]:

 @Path("/users/{username}")
public class 
UserResource {

    @
GET
    @Produces("text/xml")
    public 
String getUser(@PathParam("username"String userName) {
        ...
    }
}

This is used to tell a web-server the URI the class will handle. In the example above, the annotation sets the route to users. This tight coupling of the metadata to the class causes flexibility issues. It's not unreasonable to want to use a class that deals with users on more than one website. However, because the class uses internal metadata using annotations, it's impossible to use the class on the URI /users on one website and /members/ on another without changing the class. This is a violation of the Single Responsibility Principle as the class has more than one reason to change.

If the class is changed then significant issues are introduced with version control: Once a bug is fixed on one website, it's then difficult to copy it over to the other website as the sites are using independent branches.

The solution is to separate out the metadata from the class using any format, for example JSON to map the URI path to the class name

 {
    
"/users""UserResource",
    
"/Products""Products"
}

Using external metadata, the metadata can differ per website and the class can remain identical, allowing for easier bugfixes and sharing of resources between projects.

As Ahuja[7] writes:

My advice is not to use annotations to configure your applications. Configurations were never meant to be part of code—that's why they're configurations and not source code. So let configurations stay in the configuration files. A small productivity gain in the short term won't go amount to much when a client asks for a change in a table or a value and you tell them it's going to take five days of development, testing and deployment.

There is an ongoing debate among developers about if and when annotations should be used. However, the sole benefit of annotations is being able to edit metadata and code in the same file, the argument in favour of their use is always at the expense of flexibility. This is an debate of convenience vs flexibility. From a flexibility point of view, annotations are considerably worse than available alternatives.

It should also be noted that this only applies to annotations which adjust the program's outcome. If removing the annotations does not affect how the program works, the annotations are there for documentation and do not cause any problems with flexibility.

As noted by Walls[8], annotations also break the law of demeter by adding extra dependencies to classes:

But, as luck would have it, SearchController also transitively depends on @Searchable and any other Compass annotations I use in Document. That means that I can't compile the web module without it having Compass in its build classpath (that is, as a dependency in its POM. Even though SearchController doesn't know about or need to know about Compass!
Doesn't this violate the law of demeter?

Although this problem is specific to Java, in other languages annotations will not introduce a hard dependency.

However, there is a remaining issue of comprehension. You could move a class with @Inject or similar annotation to a project where the annotations are just comments. Anyone looking at the class in this project will assume that the annotations are used and will be surprised when they change the annotations and it doesn't affect the configuration. This is not directly an issue of flexibility but it breaks the Principle of Least Surprise[9][10] and makes the class more difficult to reuse because it's not clear to anyone reading the code.

Summary of problems:

  • Cannot be debugged easily as you can't print the contents of the configuration
  • Finding application configuration requires looking across every class in the application
  • Breaks the Single Responsibility Principle
  • Breaks Encapsulation
  • Breaks Separation of Concerns
  • Introduces ambiguity/bugs in polymorphic code
  • Introduces coupling between unrelated components
  • Makes it more difficult to instantiate an object with different configurations
  • Makes version control more difficult

References

  1. Reigler, G. (2014) An Annotation Nightmare [online]. Available from: http://www.javacodegeeks.com/2014/01/an-annotation-nightmare.html
  2. Dohms, R. (2013) Annotations in PHP: They Exist [online]. Available from: http://www.slideshare.net/rdohms/annotations-in-php-they-exist
  3. Oracle, O. (2011) Annotation Type Inject [online]. Available from: http://docs.oracle.com/javaee/6/api/javax/inject/Inject.html
  4. Bugayenko, Y. (2016) Java Annotations Are a Big Mistake [online]. Available from: http://www.yegor256.com/2016/04/12/java-annotations-are-evil.html
  5. Oracle, O. (2010) The @Path Annotation and URI Path Templates [online]. Available from: https://docs.oracle.com/cd/E19798-01/821-1841/ginpw/
  6. Symfony Framework, S. (nd) @Route and @Method [online]. Available from: http://symfony.com/doc/2.0/bundles/SensioFrameworkExtraBundle/annotations/routing.html
  7. Ahuja, K. (2015) Are Annotations Bad? [online]. Available from: https://dzone.com/articles/are-annotations-bad
  8. Walls, C. (2008) When Good Annotations Go Bad [online]. Available from: http://java.dzone.com/articles/when-good-annotations-go-bad
  9. Raymond, E. (2003) The Art of Unix Programming ISBN: 978-0131429017. Addison Wesley.
  10. James, G. (1987) The Tao of Programming ISBN: 978-0931137075. Info Books.
*/ public function setCookie(SetCookie $cookie): bool { // If the name string is empty (but not 0), ignore the set-cookie // string entirely. $name = $cookie->getName(); if (!$name && $name !== '0') { return false; } // Only allow cookies with set and valid domain, name, value $result = $cookie->validate(); if ($result !== true) { if ($this->strictMode) { throw new \RuntimeException('Invalid cookie: ' . $result); } else { $this->removeCookieIfEmpty($cookie); return false; } } // Resolve conflicts with previously set cookies foreach ($this->cookies as $i => $c) { // Two cookies are identical, when their path, and domain are // identical. if ($c->getPath() != $cookie->getPath() || $c->getDomain() != $cookie->getDomain() || $c->getName() != $cookie->getName() ) { continue; } // The previously set cookie is a discard cookie and this one is // not so allow the new cookie to be set if (!$cookie->getDiscard() && $c->getDiscard()) { unset($this->cookies[$i]); continue; } // If the new cookie's expiration is further into the future, then // replace the old cookie if ($cookie->getExpires() > $c->getExpires()) { unset($this->cookies[$i]); continue; } // If the value has changed, we better change it if ($cookie->getValue() !== $c->getValue()) { unset($this->cookies[$i]); continue; } // The cookie exists, so no need to continue return false; } $this->cookies[] = $cookie; return true; } public function count() { return \count($this->cookies); } public function getIterator() { return new \ArrayIterator(\array_values($this->cookies)); } public function extractCookies(RequestInterface $request, ResponseInterface $response): void { if ($cookieHeader = $response->getHeader('Set-Cookie')) { foreach ($cookieHeader as $cookie) { $sc = SetCookie::fromString($cookie); if (!$sc->getDomain()) { $sc->setDomain($request->getUri()->getHost()); } if (0 !== \strpos($sc->getPath(), '/')) { $sc->setPath($this->getCookiePathFromRequest($request)); } $this->setCookie($sc); } } } /** * Computes cookie path following RFC 6265 section 5.1.4 * * @link https://tools.ietf.org/html/rfc6265#section-5.1.4 */ private function getCookiePathFromRequest(RequestInterface $request): string { $uriPath = $request->getUri()->getPath(); if ('' === $uriPath) { return '/'; } if (0 !== \strpos($uriPath, '/')) { return '/'; } if ('/' === $uriPath) { return '/'; } if (0 === $lastSlashPos = \strrpos($uriPath, '/')) { return '/'; } return \substr($uriPath, 0, $lastSlashPos); } public function withCookieHeader(RequestInterface $request): RequestInterface { $values = []; $uri = $request->getUri(); $scheme = $uri->getScheme(); $host = $uri->getHost(); $path = $uri->getPath() ?: '/'; foreach ($this->cookies as $cookie) { if ($cookie->matchesPath($path) && $cookie->matchesDomain($host) && !$cookie->isExpired() && (!$cookie->getSecure() || $scheme === 'https') ) { $values[] = $cookie->getName() . '=' . $cookie->getValue(); } } return $values ? $request->withHeader('Cookie', \implode('; ', $values)) : $request; } /** * If a cookie already exists and the server asks to set it again with a * null value, the cookie must be deleted. */ private function removeCookieIfEmpty(SetCookie $cookie): void { $cookieValue = $cookie->getValue(); if ($cookieValue === null || $cookieValue === '') { $this->clear( $cookie->getDomain(), $cookie->getPath(), $cookie->getName() ); } }}