G'day:
I've had these notes lying around for a while, but they were never interesting enough (to me) to flesh out into a full article. But feature toggling has been on my mind recently (OK, it's because of Working Code Podcast's coverage of same: "018: Feature Flags (Finally!)"), so I figured I'll see what I can do with it.
BTW you don't need to listen to the podcast first. It doesn't contextualise anything I say here (well maybe one thing down the bottom), it was just the catalyst for the decision to write this up. But go listen anyway. It's cool.
I see a lot of code where the dev seems to have though "OK, I need to alternate behaviour here based on one of the toggles our FeatureToggleService provides, which is based on the request the controller receives". They are looking at this code:
class SomeDao {
SomeDao(connector){
this.connector = connector
}
getRecords() {
// lines of code
// that get records
// from this.connector
// and does something with them
// and then something else
// and return them
}
}
And the alternative behaviour is to get the records from somewhere else, process them slightly differently, but the end result is the same sorta data structure. They decide to do this:
class SomeDao {
SomeDao(someConnector, someOtherConnector) {
this.someConnector = someConnector
this.someOtherConnector = someOtherConnector
}
getRecords(someFeatureFlag) {
// lines of code
if (someFeatureFlag) {
// that get records the new way
// from this.otherConnector
// and does something different with them
} else {
// that get records the old way
// from this.connector
// and does something with them
}
// and then something else
// and return them
}
}
So the code that calls that needs to know the feature flag value to pass to getRecords. So they update it:
class SomeRepository {
getObjects(someFeatureFlag) {
records = this.dao.getRecords(someFeatureFlag)
// code to make objects from the record data
return objects
}
}
But yeah the repo method needs to know about the someFeatureFlag value now, so we update the service that calls it:
class SomeService {
getBusinessObjects(someFeatureFlag) {
// code that does some business logic before we can get the objects
objects = this.repo.getRecords(someFeatureFlag)
// other code that does some business logic after we've got the objects
return stuffTheControllerNeeds
}
}
And the controller that calls the service:
class SomeController {
SomeController(featureFlagService) {
this.featureFlagService = featureFlagService
}
fulfilSomeRequest(request) {
// some request stuff
someFeatureFlag = this.featureFlagService.getSomeFeatureFlag(basedOn=request)
results = this.service.getBusinessObjects(someFeatureFlag)
// use those results to make the response
return response
}
}
And then somewhere between the routing and the call to the controller method, there's an IoC container stitching everything together:
class SomeIocContainer {
getSomeController() {
return new SomeController(
new SomeService(
new SomeRepository(
new SomeDao(
new SomeConnector(),
new SomeOtherConnector()
)
)
),
new FeatureFlagService()
)
}
}
And we conclude we need to do that, because the controller is the only thing that knows about the request, and we need to the request to get the correct toggle value.
Hrm. But that's less than idea (no shit, Sherlock)… maybe we could give the DAO constructor the FeatureToggleService? We'd then still need to pass the request through the call stack. Deep breath:
class SomeIocContainer {
getSomeController() {
return new SomeController(
new SomeService(
new SomeRepository(
new SomeDao(
new SomeConnector(),
new SomeOtherConnector(),
new FeatureFlagService()
)
)
)
)
}
}
class SomeController {
function fulfilSomeRequest(request) {
// some request stuff
results = service.getBusinessObjects(request)
// use those results to make the response
return response
}
}
class SomeService {
getBusinessObjects(request) {
// code that does some business logic before we can get the objects
objects = repo.getRecords(request)
// other code that does some business logic after we've got the objects
return stuffTheControllerNeeds
}
}
class SomeRepository {
getObjects(request) {
records = dao.getRecords(request)
// code to make objects from the record data
return objects
}
}
class SomeDao {
SomeDao(someConnector, someOtherConnector, featureFlagService) {
this.someConnector = someConnector
this.someOtherConnector = someOtherConnector
this.featureFlagService = featureFlagService
}
getRecords(request) {
// lines of code
if (this.featureFlagService.getSomeFeatureFlag(basedOn=request)) {
// that get records the new way
// from this.otherConnector
// and does something different with them
} else {
// that get records the old way
// from this.connector
// and does something with them
}
// and then something else
// and return them
}
}
This is not an improvement. I'd go as far as to say it's worse because there's no way the request should ever leak out of its controller method.
And you might think I'm exaggerating here, but I have seen both those implementations in the past. I mean: I saw some today. And in the former - historical - cases the devs concerned sincerely went "it's not possible any other way: I need the toggle in the DAO, and the featureFlagService needs the request object to work out the toggle value".
I take the dev back to a pseudo-code version of their DAO method:
getRecords(something) {
// lines of code
if (something) {
// that get records the new way
// from this.otherConnector
// and does something different with them
} else { // something else
// that get records the old way
// from this.connector
// and does something with them
}
// and then something else
// and return them
}
The first red flag here is the else. Don't have elses in yer code. You don't need them, and they are a code smell. We extract that code:
getRecords(something) {
// lines of code
things = doTheThing(something)
// and then something else
// and return them
}
doTheThing(something){
if (something) {
// that get records the new way
// from this.otherConnector
// and does something different with them
return them
}
// something else
// that get records the old way
// from this.connector
// and does something with them
return them
}
But this makes it more clear we're going the wrong way about this. doTheThing? How many things is it doing? Two possible things. And that is based on it doing a third thing: checking which of the two things it's supposed to do. So three things. How many things is any one bit of code supposed to do? One thing. And even if we refactor that doTheThing method some more:
doTheThing(something){
if (something) {
return getThemTheNewWay()
}
return getThemTheOldWay()
}
We've still got a DAO now that's doing things in two different ways; and it still needs to know how to make that decision. It's not the job of a DAO to check feature toggles. It's the job of a DAO to get data from storage. So even with a decent clean-code refactoring, it still smells from an architectural POV.
(Oh yeah and I happen to be using a DAO in this example, but the same applies to any sort of class. It's going to be its job to doTheThing, not make decisions about how to do thing based on a feature toggle).
How are we supposed to implement feature toggling in our code? In our object oriented code? Use OOP.
Use a factory method in the IOC container to get the correct DAO for the job:
class SomeIocContainer {
getDao() {
if (this.featureFlagService.getSomeFeatureFlag(basedOn=this.request)) {
return SomeOtherDao(new SomeOtherConnector())
}
return SomeDao(new SomeConnector())
}
getSomeController() {
return new SomeController(
new SomeService(
new SomeRepository(
getDao()
)
)
)
}
}
It is precisely the job of a factory method to make a call as to which implementation of a class to use. And it is precisely the job of an IOC container to compose services. Each are doing one job.
DAOs:
class SomeDao {
SomeDao(connector){
this.connector = connector
}
getRecords() {
// lines of code
records = getRecordsFromConnector()
// and then something else
// and return them
}
getRecordsFromConnector() {
// that get records
// from this.connector
// and does something with them
}
}
This is just a refactoring of the original method to pull some implementation into its own method. It has no idea about feature toggles, or new ways of doing thing.
class SomeOtherDao extends SomeDAO {
getRecordsFromConnector() {
// that get records the new way
// from this.connector
// and does something different with them
}
}
This implementation only knows about the new way of doing things. Other than that it's the same DAO as the original. It also doesn't know anything about feature toggles.
One could put a case forward here that getRecordsFromConnector might be a member of a different class which is injected into the one original DAO. IE: favouring composition over inheritance. This is a case of "it depends", I think. And either way, that's just shifting the implementation: the mechanism is still the same.
Aside from the architectural improvements, when we come to remove the toggle, all we need to do is remove that one if statement, in an isolated and very simple factory method, nowhere near any implementation logic. That's it.
Bottom line, here are two things to think about when implementing your toggling:
- If you are needing to refer to the toggle in more than one place - passing it around, or checking it more than once - you are probably doing something in a less than ideal fashion.
- If you have the code that checks the toggle and the code that acts on the toggle in the same place, you are also probably not nailing it.
Maybe you are nailing it for the situation you are in. But you should definitely be asking yourself about your architecture and your OOP if you find yourself in either of those situations. And it's more than likely gonna be really easy to design the solution cleanly.
Oh and of course you need to bloody test your toggling. You're adding a thumping great decision into yer app. Feature toggling doesn't free you from testing, it increases your testing burden, because it's another moving part in your app. The clue is in the acceptance criteria: "it should do things this new way in this new situation" and "it should keep doing stuff the old way in the usual run of events". Those are your test cases. And by the very nature of the toggling approach, it's obviously a sitter for test automation rather than eyeballing it every time you do a release. Anyone who suggests feature toggling somehow makes their code more robust and requires less testing as a result (/me looks directly down camera at Ben) is having a laugh ;-)
And on that semi-in-joke note, that's enough out of me for today. Got tomorrow to get my brain ready for, and practising my line "just when I thought I was out…".
Righto.
--
Adam