2

I am currently reviewing a PR where we want to enable a feature based on some suscription date of a user such as:

let featureEnabled = false
if (user.createdAt > MONETIZATION_DATE) {
  featureEnabled = true
}

You get the idea. When reviewing changes like this, I usually suggest to make it not dependent on a date but on a feature of the user instead. Here is a possible rewrite following this idea:

let featureEnabled = false
if (user.referralVersion === 'V3') {
  featureEnabled = true
}

Note: The referral program is the actual use case I have

The reasons for this suggestion are:

  • We don't want the code to change it's behavior after some date automatically. It makes it difficult to reason about some behavior that was different from one user to another.
  • If we accept this kind of pattern, looking 5 years headed, we could have the whole codebase depending on 50 dates were we wanted to change something. And the associated behaviors would be scattered everywhere in the code.

However, we can argue about my points:

  • 5 years in the future is probably 4.5 years too far (we are an early-stage startup)
  • The business requires this change from the actual date so it makes sense for the code reflect this change (that is part of our terms of condition)
  • Making the switch depending on the condition requires some manual trigger the day we want to enable the feature, which is error-prone.

How do you usually tackle similar cases? Do you see other pros and cons I did not mention?

2
  • Is this about a customer's subscription date, or are you going to enable or disable features based on hard calendar dates? – Robert Harvey Jul 13 '20 at 19:14
  • Thanks, MONETIZATION_DATE is a hard calendar date, and the createdDate field is a user subscription date. Does it answer your question ? – rpechayr Jul 13 '20 at 19:32
2

For any particular feature that could apply to a user or not, you write one function that returns exactly that information.

What's inside that function is totally irrelevant, as long as it works. This check "if (user.createdAt > MONETIZATION_DATE)" is absolutely fine, if it is correct obviously, and if it is used in that one place only. And any code that needs to know if the feature is present calls that function, and it's obvious what the code does.

You want to use an intermediate property in between: "if (user.referralVersion === 'V3')". So this needs to be documented well, because this is in itself meaningless.

1
  • Thanks, I would personnaly not agree with your points. I might be wrong since I wrote the question in the first place: - What's inside a function is totally irrelevant: from a client perspective, you are right. However, this question is from a maintainer perspective. Making things work is usually not the only goal - user.referralVersion === 'V3' is meaningLess. I agree that the code the slightly more complex. However, in this particular case, the code is 100% meaningfully. The reason why we enable the feature is not just a date, it's a marketing operation with many characteristics – rpechayr Jul 13 '20 at 19:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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