DRY Violations May Indicate A Missed Modeling Opportunity
In a previous blog post, I talked about some potential options for modeling an “item selected” event and a “item de-selected” event. In that post, I suggested a couple of options and stated which one I thought was the right way to do things: model the de-selected even explicitly. The conversation in the comments was quite interesting and provided some great info on when / where / why to use both options. Fast forward almost a month and a half, and I never actually implemented the explicit modeling for the de-selected event. I left the code as it was originally, using a null value in the selected event data to signify the de-selected or “none” selected event… and now it’s come back to haunt me.
Multiple Item Selected Events Exposing A Bug
A new requirement came in that causing me to change how the code in question behaved. Rather than having a single item selected event based on the last drop down list on the screen, I now needed two item selected events: 1 for the next to last drop down list, and 1 for the last drop down list. These two events are prioritized. A simple version of the prioritization can be stated like this:
- if the user selects an item in the next to last drop down list only, then this is the data we need to use
- if the user selects an item in the last drop down list, then this is the data we need to use
This is a simplified implementation of the process, but reflects my code very accurately (I’ve only removed a few lines of code that are not pertinent to the example and in-lined one method call to keep it simple for this example). Can you spot the bug in this code based on the stated requirements?
1: private void AssetTypeSelected(Lookup assetTypeLookup)
2: {
3: AssetType assetType = null;
4: if (assetTypeLookup.Value != selectOne.Value)
5: {
6: assetType = assetTypeRepository.GetById(assetTypeLookup.Value);
7: var productCodes = productCodeRepository.GetByAssetType(assetType);
8: if (productCodes.Count == 1)
9: appController.Raise(new AssetClassified(productCodes[0]));
10: }
11: appController.Raise(new AssetClassified(assetType));
12: }
</div> </div>
(Note: The calls to the appController.Raise method are the events being fired. ‘AssetClassified’ represents the ‘selected’ event that I am referring to.)
The bug is related to lines 9 and 11. In the scenario where a user selects an asset type and there is only 1 product code available for that asset type, the system will auto-select the product code. The previous requirements stated that we should use the product code data if it is selected, however, this code will always publish the asset classification (item selected) event for the asset type last. Therefore, the product code selection will never be used for the classification.
A Quick Fix And A DRY Violation
A tester found the bug in this code recently, and I was tasked with fixing it. The quick-fix solution was easy enough:
1: private void AssetTypeSelected(Lookup assetTypeLookup)
2: {
3: AssetType assetType = null;
4: AssetClassified assetClassified = null;
5: if (assetTypeLookup.Value != selectOne.Value)
6: {
7: assetType = assetTypeRepository.GetById(assetTypeLookup.Value);
8: assetClassified = new AssetClassified(assetType);
9:
10: var productCodes = productCodeRepository.GetByAssetType(assetType);
11: if (productCodes.Count == 1)
12: assetClassified = new AssetClassified(productCodes[0]);
13: }
14: else
15: {
16: assetClassified = new AssetClassified(assetType);
17: }
18: appController.Raise(assetClassified);
19: }
</div> </div>
The thing to note is that I realized I needed to duplicate the call to new AssetClassified(assetType)); on line 8 and line 18. The reason for this is that when an asset type is selected from the drop list, I have to classify the item by the asset type before checking for a product code and whether or not I should auto-classify based on a single product code being available. If the user has selected nothing in the list, then the asset classification is de-selected. Since I have modeled de-selection by a null value, I need to ensure that I am raising the even with a null value. I can’t pass “null” directly to the AssetClassified class because it has an overloaded constructor – one for product codes and one for asset types, forcing me to specify the type via a variable that evaluates to null in this case.
The end result is duplicated code – a violation of the Don’t Repeat Yourself (DRY) principle.
Eliminating The DRY Violation With Proper Modeling
I find myself coming round full circle to the original options and ‘the right way’ that I listed in that post almost a month and a half ago, realizing that if I had modeled the de-selected event as it’s own explicit object, I could have potentially avoided this situation. At the very least, the need to separate selected and de-selected would have given me a little more insight and allowed me more opportunity to see that I was introducing a bug in the original version of the code, above. Part of the reason for this is that in my effort to eliminate DRY violations in the original code at the top of this post, I did not want more than one place where I called appController.Raise(new AssetClassified(assetType)). I put this call at the very end of the method so that it would only be done once and would have the value of null or the actual asset type value being passed into it based on the previous code in the method. It was through this effort to eliminate DRY violations that I wound up causing DRY violations when my needs changed. If I had modeled an explicit AssetDeclassified event from the start, I would have been forced to make two separate calls to the appController.Raise – one for the AssetClassified and one for the AssetDeclassified events.
Look at the following code, for example, where the asset de-classification (de-selection of an asset type) is modeled as it’s own event:
1: private void AssetTypeSelected(Lookup assetTypeLookup)
2: {
3: if (assetTypeLookup.Value != selectOne.Value)
4: {
5: AssetClassified assetClassified = null;
6:
7: var assetType = assetTypeRepository.GetById(assetTypeLookup.Value);
8: assetClassified = new AssetClassified(assetType);
9:
10: var productCodes = productCodeRepository.GetByAssetType(assetType);
11: if (productCodes.Count == 1)
12: assetClassified = new AssetClassified(productCodes[0]);
13:
14: appController.Raise(assetClassified);
15: }
16: else
17: {
18: appController.Raise(new AssetDeclassified());
19: }
20: }
</div> </div>
This code provides the same functionality as the previous DRY violation code, but does so with two distinct events on line 14 and line 18. I am no longer paying attention to null values as the indication of whether an asset has been de-classified, and I am not concerned with DRY violations between these two lines of code because they are obviously raising different events now.
The new version of the code doesn’t just provide the same functionality without violating DRY, though. I would go so far as to say is espouses additional principles in that it models the selected vs. de-selected events as first class citizens, separately, thus preventing Single Responsibility, Liskov Substitution, DRY, and other principle violations. I also gained new opportunities to simplify and clean up the AssetTypeSelected method, providing additional places to model explicit concepts as method calls within the code. For example, the contents of the positive-path in the if statement could be extracted into a “ClassifyAsset()” method call, simplifying the AssetTypeSelected method and making it easier to read. I also have to model the product code de-selection de-selection. By providing an explicit AssetDeclassified event, both the asset type and product code de-selection needs can be solved without having to resort to semantic coupling – requiring the developer to know that they should pass in a null value event when de-selecting the product code or asset type.
Explicit Modeling Is Not Just For Readability
The lesson that I’m taking from this experience is that explicit modeling doesn’t just help create code that is easier to read and understand. It can also help us prevent a number of principle violations. In this specific case it was the DRY principle violation that tipped me off to the problems, but once I started digging into it I realized that there were a number of other principles being violated as well. The original solution, while functional, was simply causing more problems and more headaches by coupling too many things together. A simple solution of modeling the two concepts, selected and de-selected, as their own events went a long way and really helped me clean up the code.