Most developers have heard about open - closed principle - one of Uncle Bob’s SOLID principles. It sounds reasonable, but it can still be a little bit blurry until the first usage on ‘live’ code. Full state of principle is: software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.
We came across a development problem that has shown us what an open-closed principle really is about. In one of our web applications we had a form with two sections (among others):
Users can add as many filters as they wish, but there are some rules - filter availability depends on chosen channels.
Demand channels: AD_EXCHANGE, HEADER_BIDDING, RESERVATION, OTHER Dynamic filters(dimensions): website, ad_unit, geo, creative_size, device
This article is mostly about code refactor, so there will be a lot of code snippets below. I tried to reduce it, but some amount of code is necessary to show code refactoring. You don't need to understand every small part of code to get the main idea.
class ResearchFormStateUpdater {
update () {
(...)
this._updateDynamicFilters();
}
_updateDynamicFilters () {
$('.dynamic-filter').each((_, filter) => {
$(filter).trigger('dynamicFilter:disableWebsites', this._shouldDisableWebsitesFields());
});
}
_shouldDisableWebsitesFields () {
return this._shouldDisableFields(ResearchFormStateUpdater.WEBSITE_DISABLING_DEMAND_CHANNELS);
}
_shouldDisableFields (disablingDemandChannels) {
// is any of disablingDemandChannels checked ?
}
}
ResearchFormStateUpdater.WEBSITE_DISABLING_DEMAND_CHANNELS = ['header_bidding', 'reservation', 'other'];
class ResearchDynamicFilter {
_setDynamicFilterDisableWebsitesEvent () {
$(this._getBody()).on('dynamicFilter:disableWebsites', (event, shouldDisableWebsites) => {
// disable website filters
});
}
}
As you can see, the website filter is supposed to be unavailable for HEADER_BIDDING, RESERVATION and OTHER channels, so it is available only for AD_EXCHANGE channel.
The last thing you can say about code is that it is permanent or static. So we have more requests from our client making these classes bigger and more complex.
Add another channel - EBDA (Website filter should be unavailable while EBDA is chosen):
expand DISABLING_DEMAND_CHANNELS by EBDA demand channel
a lot of name changing - in the first implementation, we specified website in methods and constants names. For example:
Spoiler alert -> when a component is open for changes, there will be a lot of name changing in the future. We won't pay attention to this in the next steps.
Add another filter for 'Product' (Product filter availability scheme is same as Website)
Let’s go bigger and add some switcher above channels -> ‘Source’. All demand channels we were having until now are in Ad Manager source. New source - SSP - has no demand channels and the only available filter is website.
Rules:
Implementation:
When 'SSP' is chosen:
When Ad Manager checked:
Add another filter for 'Platform'
Rules:
Difficulty:
I present you next snippet mainly to show code complexity. Feel free to leave it hidden.
class ResearchFormStateUpdater {
update () {
(...)
this._triggerCallbacks();
}
_triggerCallbacks () {
// choose callbacks depending on source
}
_adManagerSourceCallbacks () {
(...)
this._enableDemandChannels(ResearchFormStateUpdater.AD_MANAGER_DEMAND_CHANNELS);
this._updateDefaultStateOfDynamicFilters();
this._updateAdManagerDynamicFilters();
}
_sspSourceCallbacks () {
(...)
this._removeDemandChannelsActiveClassAndDisable(ResearchFormStateUpdater.AD_MANAGER_DEMAND_CHANNELS);
this._updateDefaultStateOfDynamicFilters();
}
_updateDefaultStateOfDynamicFilters () {
$('.dynamic-filter').each((_, filter) => {
$(filter).trigger('dynamicFilter:enableSspFilters', this.isSourceSsp);
});
}
_updateAdManagerDynamicFilters () {
$('.dynamic-filter').each((_, filter) => {
$(filter).trigger('dynamicFilter:disableWebsitesAndProducts', this._areFormStateDimensionsDisabled() && !this.isSourceSsp);
});
}
_shouldDisableFields (disablingDemandChannels) {
// is any of disablingDemandChannels is checked
}
}
ResearchFormStateUpdater.AD_MANAGER_DISABLING_DEMAND_CHANNELS = ['header_bidding', 'reservation', 'other', 'ebda'];
class ResearchDynamicFilter {
// I didn't simplify those two methods body to show current implementation complexity
_setDefaultDynamicFiltersToggleEvent () {
$(this._getBody()).on('dynamicFilter:enableSspFilters', (event, shouldEnableSspOptions) => {
this._setDefaultFiltersOptionDisabledState(shouldEnableSspOptions);
const selectedFilterDimension = this._getFiltersDimension().find('option:selected').val();
if (selectedFilterDimension === 'website') {
this._toggleChosenFilterDisabledState(false);
} else if (selectedFilterDimension === 'platform') {
this._toggleChosenFilterDisabledState(!shouldEnableSspOptions);
} else {
this._toggleChosenFilterDisabledState(shouldEnableSspOptions);
}
});
}
_setDynamicFilterDisableWebsitesAndProductsEvent () {
$(this._getBody()).on('dynamicFilter:disableWebsitesAndProducts', (event, shouldDisableWebsitesAndProducts) => {
const selectedFilterDimension = this._getFiltersDimension().find('option:selected').val();
if ($.inArray(selectedFilterDimension, ['website', 'product']) >= 0) {
this._toggleChosenFilterDisabledState(shouldDisableWebsitesAndProducts);
}
this._setMethodSelectWebsiteAndProductOptionDisabledState(shouldDisableWebsitesAndProducts);
});
}
_toggleNonSspFilters (dimensionSelect, shouldDisable) {
$.each(ResearchDynamicFilter.NON_SSP_FILTERS_OPTIONS, (_, option) => {
// toggle filter state depending on 'shouldDisable'
});
}
}
ResearchDynamicFilter.NON_SSP_FILTERS_OPTIONS = ['ad_unit', 'creative_size', 'geo', 'device', 'product'];
We still use some 'toggle' mechanism. It is really hard to switch 4 levers and get to expected state and now DynamicFilter has to know, which dimensions are not for ssp source.
We do have ResearchFormStateUpdater, why shouldn’t it be in charge?
That is the exact moment when we decided to refactor those classes. Channels and filters being analyzed are just a small part of the problem. There are multiple form sections here and all of them have the same problem. Our refactor should neutralize the need for changing inside methods of those classes *to* add some new channels or dimensions.
In the next snippet, I left the main classes almost as they are in our production code to show you how easy to understand they are now.
class ResearchFormStateUpdater {
update () {
(...)
this._updateDynamicFilters();
}
_updateDynamicFilters () {
this._toggleAllDynamicFiltersState(this._dynamicFiltersDimensionsToBeDisabled());
}
_dynamicFiltersDimensionsToBeDisabled () {
if (this.isSourceSsp) { return ResearchFormStateUpdater.NO_SSP_FILTERS; }
var disabledFilters = ResearchFormStateUpdater.ONLY_SSP_FILTERS;
if (this.areDemandChannelsExceptAdxSelected) {
disabledFilters = disabledFilters.concat(ResearchFormStateUpdater.ONLY_ADX_FILTERS);
}
return disabledFilters;
}
_toggleAllDynamicFiltersState (disabledFilters) {
$('.dynamic-filter').each((_, filter) => {
this._toggleDynamicFilterState(filter, disabledFilters);
});
}
_toggleDynamicFilterState (dynamicFilter, disabledFilters) {
$(dynamicFilter).trigger('dynamicFilter:toggleDynamicFilters', disabledFilters);
}
}
ResearchFormStateUpdater.NO_SSP_FILTERS = ['ad_unit', 'creative_size', 'geo', 'device', 'product'];
ResearchFormStateUpdater.ONLY_SSP_FILTERS = ['platform'];
ResearchFormStateUpdater.ONLY_ADX_FILTERS = ['website', 'product'];
class ResearchDynamicFilter {
_setDynamicFiltersToggleEvent () {
$(this._getBody()).on('dynamicFilter:toggleDynamicFilters', (event, disabledFilters) => {
this._disableFilters(disabledFilters.split(','));
this._enableFilters(disabledFilters.split(',')));
});
}
_disableFilters (filtersToDisable) {
// disable filtersToDisable
}
_enableFilters (filtersToDisable) {
const filtersToEnable = $(ResearchDynamicFilter.ALL_FILTERS).not(filtersToDisable).get();
// enable filtersToEnable
}
}
ResearchDynamicFilter.ALL_FILTERS = ['website', 'ad_unit', 'creative_size', 'geo', 'device', 'product', 'platform'];
Now the only thing 'ResearchDynamicFilter' has to know is a list of all filters - seems fair. The rest of logic and control comes from above - some higher methods and constants.
So let's try out our new structure with adding a filter for 'Yield_partner':
class ResearchFormStateUpdater {
_dynamicFiltersDimensionsToBeDisabled () {
(...)
if (this.areDemandChannelsExceptEbdaSelected) {
disabledFilters = disabledFilters.concat(ResearchFormStateUpdater.ONLY_EBDA_FILTERS);
}
return disabledFilters;
}
}
ResearchFormStateUpdater.NO_SSP_FILTERS = [(...), 'yield_partner'];
ResearchFormStateUpdater.ONLY_EBDA_FILTERS = [(...), 'yield_partner'];
ResearchDynamicFilter.ALL_FILTERS = [(...), 'yield_partner'];
As you can see, it is all about adding some values to constants and some additional conditions.
Thanks to ‘open-closed principle’ we are able to change business logic of form with only adding some values and conditions on a higher level of abstraction. We do not need to go inside the component and change anything. This refactor affected the whole form and there were more sections and they all obey the open-closed principle now.
We didn't reduce the amount of code - as a matter of fact, we even increased it (before/after):
It's all about the collection in constants -> it's our public interface now, our console to control process without tens of switchers.
Read also:
