Should constructors ever be used only for side-effects?

Question

Summary: Why is it wrong to design a constructor only for its side effects, and then to use the constructor without ever assigning its return value to a variable?

I'm working on a project that involves modeling card games. The public API for the game uses an odd pattern for instantiating Rank and Suit objects. Both use a static list to track instances. In a standard deck, there will only ever be four Suit and thirteen Rank objects. When constructing a card, Ranks and Suits are obtained with a static getRank() and getSuit(), which returns the identified item from the static list. Rank and Suit have other static methods such as removeRank() and clear().

What is odd is that there is no static add() method to add a new Rank or Suit to the static list. Instead, this behavior occurs only when a Rank or Suit constructor is invoked via new. Since instances of Rank and Suit are obtained with the static getRank(), the return value from the new constructors is completely irrelevant.

Some sample code might look like this

//set up the standard suits
Suit.clear(); //remove all Suit instances from the static list
new Suit("clubs", 0);  //add a new instance, throw away the returned value
new Suit("diamonds", 1);
new Suit("hearts", 2);
new Suit("spades", 3);

//create a card
Deck theDeck = new Deck()
for (int i = 0; i < 4; i++) {
    for (int j = 0; j < 13; j++) {
        Suit theSuit = Suit.getSuit(i);
        Rank theRank = Rank.getRank(j);
        theDeck.add(new Card(theSuit, theRank));
    }
}

I've never seen code that didn't assign the object being returned by a constructor. I didn't even think this was possible in Java until I tested it. However, when the issue was raised that the static addSuit() method was missing, the response was

"Objects of type Suit and Rank are created via constructors. The intent is to mimic the behavior of the java.lang.Enum class in which objects are created via constructors and the class maintains the collection of such objects. Current behavior is designed as intended."

Using constructors in this manner smells really wrong to me, and I want to reopen the issue, but I can't seem to find much material in support (coding guidelines, anti-patterns, other documentation). Is this in fact a valid coding pattern that I just haven't come across before? If not, what is a good argument I can make for getting the API changed?

Here is an article that talks about what constructors are expected to do:

We can agree that a constructor always needs to initialize new instances, and we might even agree by now that a constructor always needs initialize instances completely.

Edits: Added additional information about designer's response to issue. Added a few articles that talk about what constructors should and should not do.

Answer 1

Sure, technically this can work, but it violates the so-called Principle of least astonishment, because the typical convention for how constructors are used and what they are good for is quite different. Moreover, it violates the SRP, since now the constructors do two things instead of the one they are usually supposed to do - constructing the object and adding it to a static list.

In short, this is an example for fancy code, but not for good code.

Answer 2

I see no sensible reason for this "pattern". As you say, the desired functionality would better be implemented by a static function; while it's possibly/probably true that using a constructor to do this doesn't actually produce any functional issues, it breaks the Principle of Least Surprise: like you, I look at that code and think "Huh!? What's going on here then?". I see no advantage to doing it this way over having a static function.

Stepping back a bit further the global nature of the suits is also a code smell: what happens if you want to model both bridge and (say) tarot, with its different suits in the same program? A better design would involve a SuitCollection or similar.

What can you do about it? That depends a lot on context. If this is a commercial project, talk to your peers to see if there's a good reason for this decision, and if not go and talk to whoever have you that response in person and try and discuss things. If it's an open source project or similar, my personal advice would be "run away" - poor quality code combined with poor communications isn't going to make for a fun project.

Answer 3

First off, let's say that one of the main goals of software design is mantainability - making sure that code is easy to understand and modify.

Next, let's say that constructor is a special type of method that initializes a newly created object to a valid state.

In the light of these two assumptions, it's safe to say that it's invalid to have a side-effect constructor in a proper design. Constructor by its nature should not have any effects besides on the newly created object and any objects it encloses.

Given this code:

public void MyMethod()
{
    DoSomething();
    new MyClass();
    DoSomething();
}

Would you find it easy to mantain? Can you reorder MyClass() and DoSomething()? You don't know until you visit MyClass constructor and see what it does, right?

This way, you'll soon find yourself having to understand the whole program in order to modify small parts of it. This is the exact opposite of good design.

Besides that, having a side-effect constructor usually implies there is something static going on in the background, which is again a red flag when it comes to testing and proper design. While seemingly cheap and manageable, static context grows very fast and hampers future program modifications.

Obviously, programming is a work of compromise and if presence of static collection of card types would achieve vast improvements in time-to-market (I have very strong doubts about that), it would still be much better to see the intent expressed via static methods and one-time initialization:

public void ApplicationSetup()
{
    Suit.Initialize("clubs", "diamonds", "hearts", "spades"); // throws if called more than once
}

However, even this way, you can end up calling parts of program that require Suit to be initialized before the initialization actually takes place. The best way to express "this class/method requires Suit to be initialized" is to take Suit as a parameter. Which is opposed to the whole static Suit idea.

Answer 4

No! It is a bad code! It is a confusing pattern which provide no benefit compared to doing it in the straightforward way.

The constructor is just used as a fancy static method. Calling it as Suit.addSuit("spades", 3); would have same effect without being confusing and return a useless instance.

The comparison to Enum's does not justify the strange use of constructors - enums does not use this pattern. With enums, constructors are used to create values of the type (e.g. as alternative to Suit.getSuit()), not to define the possible options, which happen through a dedicated syntax.

Furthermore, the use of Suit.Clear() indicates this is not at all equivalent to Enums. The point of enums is the available values are defined and fixed at compile time and therefore can be statically checked. The Clear() method indicates the options will vary at runtime. If you have different sets of suits at runtime, it should not be defined as a static collection at all.

Answer 5

I agree with the other answers that this pattern is poor and violates various software engineering principals. Apparently your coworkers don't care much about them. What you need are example use cases where this pattern fails or is awkward to use. Here's a couple:

1. The game is Bridge. After bidding, sometimes you must change the name of one of the four existing suits to "trump". Try writing that code. I predict it will be icky, buggy, and may even break the existing code.
2. The game is Hearts. Rename the Queen of Hearts to "The Lady". (PG version).
3. Your game is a huge hit in China and Armenia. Add a menu for language that they can pick anytime during tha game. You must get this version to market before your competitors do! Oh yeah, its a multiplayer game and the big Armenia vs. Shanghai match is coming up, with a cable TV audience in the USA.

Answer 6

In addition to all these valid concerns, there is one exception I dare to add: sandwiches in C++.

The sandwich is a pattern like

initialize()
do_stuff()
cleanup()

In many languages this pattern is supported by a built-in construct (try-with-resource in Java, context managers in Python, IDisposable in .NET). In C++, this is implemented with the "RAII" idiom (Resource Acquisition Is Initialization). In this idiom, a resource is acquired by construction and released by destruction of a dedicated object.

C++ was created with 'deterministic garbage collection': objects are destructed the moment they go out of scope. This way the constructor/destructor pair can be used to implement the sandwich.

class OpenFile {
public:
  OpenFile(const char *filename): f(fopen(filename)) {
    if (!f) { throw std::runtime_error("could not open file"); }
  }
  ~OpenFile() { close(f); }
  FILE * f;
};

...
{
  auto log = OpenFile{"program.log"};
  fprint(log.f, "written a line\n");
  // automatically closed!  Even when an exception occurs
}

As long as we're talking C++, the class is named properly, and the constructor does exactly this one thing, I'm happy to accept this during code review.

But it still is surprising when you don't know it, so people have created clarifying wrappers for this, too (e.g. BOOST_SCOPE_EXIT)