2

In Java's documentation, it states:

Don't allow subclasses to override methods. The simplest way to do this is to declare the class as final. A more sophisticated approach is to make the constructor private and construct instances in factory methods.

From Clean Code(Page 25):

When constructors are overloaded, use static factory methods with names that describe the arguments

For example:

Complex fulcrumpoint = Complex.FromRealNumber(23.0);

is generally better than

Complex fulcrumPoint = new Complex(23.0);

But according to the comments to this answer, when it comes to this code:

private Weapon(String name, int damage)
{
    this.name = name;
    this.damage = damage;
}

public void attack(Enemy enemy){ // code to cause damage to enemy }

public static Weapon Sword(String name, damage){
    return new Weapon(name, damage);
}

public static Weapon Sniper(String name, damage){
    return new Weapon(name, damage);
}

It'll compile and execute, but it violates the Principle of least astonishment.

Did the author of Clean Code violate the Principle of Least Astonishment to write what is considered clean code?

Christophe
  • 74,672
  • 10
  • 115
  • 187
  • I think there's some context lacking from your example. In a vacuum, no, I don't think adding these static methods is worth it and may violate the Principle of least astonishment, but I'm sure we can come up with scenarios where it's worthwhile. That being said, you can't say that adding static factory methods always violate the POLA. [`Duration`](https://docs.oracle.com/javase/9/docs/api/java/time/Duration.html) is a great example of this in the Java world. – Vincent Savard Mar 02 '18 at 17:48
  • 1
    "When constructors are overloaded" – Stop harming Monica Mar 02 '18 at 21:09
  • 3
    The POLA violation here *isn't* using static methods to construct the obejcts: after all, that's a very common approach and nobody should be even remotely astonished because of it. The reason why anyone would say this violates the principle is that *you have two different methods that do exactly the same thing*. If you had a single `public static Weapon withNameAndDamage(String name, int damage)`, that'd be fine, and nobody would care in the slightest. – Jules Mar 02 '18 at 21:50
  • 1
    I think this is not a good example. If Sword ("x", 100) and Sniper ("x", 100) return the exact same object I'd find that astonishing. – gnasher729 Mar 03 '18 at 13:19

4 Answers4

4

Also from the link:

@S.R.: no, it's not fine. It'll compile and execute, but it violates the Principle of least astonishment. If you want to create an object, you should normally use new.

This seems like very outdated advice. These days it is very common to use factory methods or dependency injection to get a new instance of something. That being said, static factories are out of fashion; you should inject an instanced factory with an interface, and set the IoC container to single instance.

John Wu
  • 26,032
  • 10
  • 63
  • 84
  • 2
    Indeed ! The weakness of POLA is that the degree of astonishment depends heavily on the observer, making the application of this principle rather a subjective matter – Christophe Mar 02 '18 at 18:14
4

No, he didn't.

The point of the advice to avoid new SpecificType() is that there is no way of using a constructor without hardcoding the exact class you want to use. Because coding to interfaces rather than implementations is a very good idea, coupling yourself to one particular concrete class is usually a bad idea. This is one of the reasons why factories and factory methods are so popular: only the factory has to change when you invent a new ImprovedSpecificClass, not the client code. (There are others, e.g. the possibility of having more informative names for creation methods than SpecificClass(), but I consider this the primary benefit.)

The example, however, shows code within the class Weapon that calls a Weapon constructor. There is no way of decoupling Weapon from itself, so there is no point in avoiding new. Doing so would be a sign that someone understood the letter but not the spirit of factory methods.

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
  • 1
    Given the *Clean Code* quote in the question, I'd say that in context the informative name is the benefit being discussed. – Jules Mar 02 '18 at 21:53
0

The quote you provided contains an important qualification:

When constructors are overloaded [...]

When you have just one obvious way to instantiate an object, a simple constructor will do just fine. But if you have a number of different ways to create the object, a named constructor can often communicate the intent better than having the user search for the matching constructor overload.

This is not an uncommon pattern. As has been stated, the Weapon example is a little weird, because both factory methods do exactly the same thing.

doubleYou
  • 2,737
  • 1
  • 11
  • 25
  • I should point out, that it's a mistake let me correct it. Right now, it won't let me fix it. –  Mar 02 '18 at 23:39
  • What's a mistake? – doubleYou Mar 02 '18 at 23:45
  • instead of having two methods that do the same thing it should read `public static Weapon sword(name){return new Sword(name,50);)` The damage is set by the class not the client code. –  Mar 02 '18 at 23:47
  • I see. In this case, constructor overloading would not have been an option anyway. My point would be that it can make sense to be explicit even if overloaded constructors could work. – doubleYou Mar 02 '18 at 23:57
  • see this question for a better idea of what I'm asking. https://stackoverflow.com/questions/49078916/use-factory-methods-to-construct-immutable-objects. Note the differences in the code example, the damage is set by the class. –  Mar 02 '18 at 23:58
0

I'd like to answer to one specific part of code from your question:

public static Weapon Sword(String name, damage){
    return new Weapon(name, damage);
}

public static Weapon Sniper(String name, damage){
    return new Weapon(name, damage);
}

These two factory methods violate the Principle of Least Astonishment in some serious aspects:

  • They don't follow the Java naming conventions (method names should begin with lower case).
  • What they do, has nothing to do with their names: they both produce unspecific Waepons, and there's nothing about Swords or Snipers in them.
  • They are two methods that do exactly the same job.
Ralf Kleberhoff
  • 5,891
  • 15
  • 19