1

There's lots of questions about the open-closed principle here, but none that I found satisfied this particular problem. At $job, I'm teaching my team Rust, but I believe the nature of this question extends beyond Rust. We have a training problem that we use to kick the tires on new ideas, because we can all imagine a few canonical solutions to it. One small aspect of this problem is that an embedded target needs to communicate with a host PC over a serial port. Naturally, I used Rust's famous enums to describe the protocol (domain terms have been erased from the example):

enum Message {
    SetCharacteristic {
        id: u8,
        characteristic: Characteristic,
    },
    SetIsEnabled {
        id: u8,
        enabled: bool,
    },
}

My mentor disapproved of this solution, because enums are closed to the addition of new variants under the open-closed principle (SO answer for reference), and we expect to receive new requirements in the future that require us to add new message types. While I absolutely agree that this is the case, refactoring this enum out to apply the OCP invariably produces a solution where message handlers are dynamically registered at runtime, so the call site goes from a match expression:

match message {
    Message::SetCharacteristic { .. } => todo!(),
    Message::SetIsEnabled { .. } => todo!(),
}

To something like this:

fn is_set_characteristic(message: &Message) -> bool { todo!() }

let mut router = Router::new();
router.add(is_set_characteristic, move |message| {
    // Handle SetCharacteristic message...
});

router.handle(message);

I think the tradeoff here is twofold:

  1. Loss of some type safety: match expressions that don't handle all representable cases are compiler errors in Rust, whereas "forgetting" to register a message handler in the second case is a runtime error.
  2. The second case asks more from the reader: it may be harder to understand.

I think the benefit of this refactor is only realized in cases where the software lifecycle of message types necessitates change independent of the protocol implementation (e.g. the Router is authored and released by ACME Corp., while my organization sets the message types for our product), but my team is very dedicated to the SOLID Principles. In a situation like this where the application of the OCP invariably reduces type-safety, how do I choose? If I choose type safety, how do I justify my choice to experienced architects and engineers?

2
  • 3
    The Open-Closed principle is appropriate when you have a library or framework published for use by other teams/organizations. In that case you don't want a change to the library to risk breaking existing code among its users. Like all design principles the OCP applies to a specific context and may cause harm when applied outside of the appropriate context.
    – JacquesB
    Commented Jul 3 at 6:41
  • 2
    Your mentor doesn’t understand the open/closed principle. It’s the whole idea to design your class so that the behaviour of instances can be changed without changing the source code.
    – gnasher729
    Commented Jul 3 at 10:58

2 Answers 2

6

I'm not a Rust developer but OCP is something that is mostly language agnostic, so I'm tackling it from that perspective.

Contradiction

There's a disconnect here.

because enums are closed to the addition of new variants under the open-closed principle

But from the linked reference you provide, it repeatedly states that OCP doesn't apply to enums:

The answer is no as OCP doesn't apply (and cannot apply) to Enums.

I agree with the linked answer, OCP just inherently does not apply to enums.


"Closed" has different meanings

The main point of confusion here seems to be what it means for enums to be a "closed" set and how that applies to a living, breathing codebase.

A closed set is a list of values where there are no valid unspecified values, e.g. a deck of cards has 4 suits and is not going to magically come up with a fifth suit on the fly.

However, this is a compile-time consideration. It is not a consideration from one compilation to the next. There is nothing inherently wrong with an enum being extended later on, when the business requirements have changed.

For example, you might extend a Gender enum to include non-binary values which didn't previously exist. Or a Status enum could start including an Intercepted status when introduction interception as a new application feature.

Enums are not "create once and never adjust", which is what your coworker is suggesting. Enums are simply a list of values that at compile-time represent a complete set of values, without considering any other magical unspecified values to be valid values for the enum. It's perfectly possible, however, that this list of values in today's compilation is different from a compilation you did last year. The closed nature of the set does not prevent that from happening in any way.

In short, word "closed" has very different meanings in "an enum is a closed set" and "an object should be closed for modification". Your colleague's feedback is incorrect. Potentially, they might still be flagging an actual issue, but "adding values to enums violates OCP" is not a correct explanation.


Caveat

I do want to address one caveat here where extending an enum would be indicative of an OCP violation. Sometimes, people make enums that get used as a way to sequentially list different pieces of logic.

For example:

public enum Operation
{
  Addition,
  Subtraction,
  Multiplication
}

If you were to add Division to this at a later stage, yeah that would most likely be an OCP violation. Not because an enum is a closed set, but because the implied use case of this enum suggests that your codebase somewhere will contain something along the lines of:

switch(calculat.Operation)
{
    case Operation.Addition:
        return person.Age + effect.Modifier;
    case Operation.Subtraction:
        return person.Age - effect.Modifier;
    case Operation.Multiplication:
        return person.Age * effect.Modifier;
}

Having to add the switch case here to now also implement Division would be an OCP violation. Not because it's related to an enum, but because that switch case is an ever-growing list of implementations which should be properly abstracted.

The solution here would be to move away from the enum type and into a proper object, so that you can polymorphically write code that does something like this:

public double CalculateThing(Guid personId, Guid effectId, Calculation calculation)
{
    var person = GetPerson(personId);
    var effect = GetEffect(effectId);

    return calculation.Calculate(person.Age, effect.Modifier);
}

Notice how this code does not have to change just because you introduce class Division : Calculation {} as a fourth kind of calculation.

This is what it means to be OCP-compliant, the CalculateThing logic did not need to be modified just because you extended your codebase to allow for a new Operation.

In the switch case scenario, it was already an OCP violation before you decided to add a new value to the enum. Adding a value to the enum was not the source of the OCP violation (the switch case was the offending code), it simply made the OCP violation stand out more because it was causing an active problem.

10
  • Thank you for this very thoughtful answer! W.r.t type safety in this context: In your example, how do you reconcile the implementation of a Calculation factory with an application like parsing? There must, fundamentally, be a big switch statement or match expression somewhere that constructs Calculations based on the value of some primitive token type. If my parser needs to support new token types in the future, that conditional logic will grow. We would say that conditional logic presents an OCP violation...but it's ultimately unavoidable, right?
    – amateurece
    Commented Jul 3 at 2:49
  • @amateurece: Honestly, that question is too far removed from the original question. There are plenty of ways to skin that cat unrelated to the general point of what constitutes an OCP violation in terms of an enum. There's no specific answer I can give that would apply to your specific use case base solely on the information provided here. This could be preconfigured data from a store, provided by a user, ... Is it possible you'll have a mapping somewhere? Sure. But it'd be a single source of well defined truth with a single responsibility, not a switch case hardcoded into the core logic.
    – Flater
    Commented Jul 3 at 3:07
  • 2
    @amateurece Also to slightly reframe your expectation, OCP doesn't so much tell you that you can't ever edit something. It suggests that you, to the best of your ability, design your code in a way that future extensions don't come with a checklist of things you now have to update in light of that new extension. That checklist will be a constant source of bugs when someone forgets to update one of the locations, and it's not obvious to a new developer which locations are on that checklist based on a look at the codebase; hence why you're suggested to steer away from this kind of code structure.
    – Flater
    Commented Jul 3 at 3:10
  • I should perhaps clarify my question, because that's actually the meat of my question! The problem in my example is a parsing problem, for a language over a serial port. I apologize if that wasn't clear. This is interesting because an enum in Rust is not like an enum in other languages. A Rust enum is an algebraic data type, which makes it much more like a tagged union. Does the OCP apply to Algebraic Data Types, is perhaps a more succinct question.
    – amateurece
    Commented Jul 3 at 3:18
  • 1
    @amateurece: This is why I'm saying it's a different question. Your coworker seems to be arguing OCP violations based on the "closed" nature of an enum set; which is a conflation of two unrelated definitions of the word "closed". That specific question is language agnostic. How you should handle the implementation of a closed set of handlers in Rust is a different question. They're both relevant to you, but that doesn't make them the same question.
    – Flater
    Commented Jul 3 at 3:57
0

If I choose type safety, how do I justify my choice to experienced architects and engineers?

There isn't any trade off to be made. A principle is something that it is followed, it is an informal agreement. When there is an obligation, a constraint, an enforcement, the need for justification to do something for the reason a principle states it it is not a principle anymore it is a rule, a restriction, a limitation that it has to be complied to and then it can be talked about exceptions from the rule. In a team that follows principles challenging the principle takes the challenger out of the team not by malice, the challenger is taken out of the team by the act of challenging an internal value that is a cue that the challenger feels different than the team feels. Although there can be collaboration there isn't a single team anymore. Principles are not written, the fact that there are written principles it is a statement saying something similar to "we feel this, if you don't mind your way". When something is felt though the technical knowledge impedes to do what it is felt the natural choice is to request support making it clear that there is a lack of technical knowledge otherwise it can be perceived being a challenge.

Loss of some type safety: match expressions that don't handle all representable cases are compiler errors

According to the quote neither of the solutions, using enum nor using message handlers, are open for extensions close for modifications. The simples, the simplest not the single one, implementation in consensus with open close principle is using an asociative array that maps types to handlers leveraging the dynamic choice of handlers based on types.

1
  • According to the quote neither of the solutions, using enum nor using message handlers, are open for extensions close for modifications. The simples, the simplest not the single one, implementation in consensus with open close principle is using an asociative array that maps types to handlers leveraging the dynamic choice of handlers based on types. I believe you described my second solution. The choice to use an associative array in the implementation of the Router is a detail, and the router.add method is the operation that associates a handler with a predicate that matches on type.
    – amateurece
    Commented Jul 3 at 12:18

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