Real Scenario

Our software development department once received a request from an administrative department for a custom verification system. This administrative department managed a number of business objects which could be placed into groups. The groups were important for the operation of the business objects and the contents of the group had far-reaching effect beyond the scope of the administrative department. There were strict requirements for which business objects could belong to which groups. These requirements were maintained by the administrators. The software allowed business objects to be put into groups as specified by the administrators. Over time, as the department for managing the groups grew and the rules for determining which objects qualified for the groups became more complex, the groups were found to contain business objects which did not qualify for them. This was making the groups unreliable and causing problems outside of the administrative department. Our development department was given a request to implement an automated solution to the problem.

We formulated a plan with the requesting administrators to create a system which would allow a certain knowledgeable sub-set of the administrators to define requirements on a group. These requirements would be automatically run against the business objects. If these requirements were not satisfied by a business object, it would not be allowed into the group. I was tasked with creating the ability to allow the administrative users to define the automated requirements on groups.

Example

I will start by taking this example from a vague definition of terms (“Business Object”, “Administrators”, and “Requirements”) into a concrete example.

Let’s view this problem through the scope of a grocery store. We are tasked with taking food from a delivery truck and putting it onto different carts which are taken to departments in the store where stock clerks put them on shelves (If you’ve worked at a grocery store, by now you’ve probably noticed that I have not. Bear with me please)

In the initial legacy solution to the problem, we open up the food boxes as they come off the truck and make judgements based on observations about the food

“It was shipped frozen. It should probably go to frozen goods”

“This one says ‘Milk’ on it so it should probably go to the dairy section”

After observing the food and deciding, based on internal criteria that was given by training, we take food from the delivery crates and put it onto carts. Occasionally the stock clerks will return carts with a few items that could not be stocked because they recognized that it didn’t belong to their department

“This is evaporated milk. It goes to the canned goods department”

“But it says milk right on it?”

“Yeah, but it’s canned”

“Sorry about that. I’m new”

As mislabelling problems are brought up by stock clerks, that information must be shared between the people responsible for sorting the food as it comes off the truck. Even if this information exists in the form of documentation, it takes action in the form of training. This leaves too much room for human-error. It is likely that the stock clerks will have to deal with mislabelled food as people are hired, move on to new jobs, or department definitions drift.

The software solution would replace the truck-unloading workforce with a food-sorting machine and a few knowledgeable administrative individuals to maintain the department rules. If a stock clerk comes back with a mislabelled product, the administrator adjusts the rules accordingly. This would reduce the training burden and create codified definitions for the food departments. This would also prevent the grocery store from losing key information through employee turnover.

Initial Solution

The administrators, at our prompting, provided us with flow-chart definitions of the group requirements. These flowcharts allowed us to determine exactly which data-points in the business objects that the process should care about and in what way we would be interacting with those data-points. Although the information was provided as a flow-chart, it later became apparent that sequence had no effect on the qualification process.

A structure was devised which took each data-point and assigned it a Field. The Field contained a Data Type, a label, and the means by which the datapoint could be located. The concept of Matcher was also created. A Matcher would take a value (located via a field), the field Data Type, and a set of Matcher Values (provided by the administrator). Based on the implementation, the Matcher would determine if the Field value was acceptable.

The administrators wanted to know which business objects were “close” to being qualified for a group as well. To this end, I implemented the matchers so that they would return a distance. It was my intention to use a Hamming Distance for determining how close something was to meeting qualifications. I had also considered allowing weights to be given to individual fields (For example “Refrigerated” could be weighted higher than “Canned” which could be weighted higher than “Milk” so that canned, non-refrigerated milk would be farther from the dairy department than any other refrigerated good) Weighted fields were not needed, so it was not implemented.

Matcher values were provided from the user. Internally, all Matcher values were stored as text. However, not all of the Matchers wanted to compare simple text values. Among the field types were date, floating point number, integer, and text. These required subtle differences when it came to matchers like “Equal”, “Greater Than”, etc. I created the Matcher so that it would take a raw untyped value from the field and the text values from the provided matcher values. The matcher would use a type provided by the field and coerce the raw field value and the matcher values before performing a comparison operation.

Initially I was concerned with certain implementation quirks such as imprecision in floating point numbers (IE 0.200000000012312 does not equal 0.20) but later a colleague pointed out to me that I was comparing a value located from the database stored in decimal form 9, 4 (4 points of precision) and matcher values provided by the user as text, so my worry was without merit. (Hindsight’s 20/20 eh?)

A screen was created to allow the administrative users to create conditions on fields and string them together into requirements which the business objects must satisfy to qualify for a group.

Everything was grand. The conditions were working as expected and the initial response of the administrative users was positive.

The Change Request

The screen for managing conditions had been in the hands of our primary requesting administrator for some time when we had a demo to show the screen to other administrative users. In this demo, we encountered a small change request. One of the users asked if the matcher values that the administrator provides would be case sensitive. The answer at the time was yes. They thought that requirement was a bit nit-picky, and I agreed. I took a not to make the change. The nature of our conditions did not require case-sensitivity and it would be unhelpful for the administrators. Another concern that was voiced was that the matcher values honored leading and trailing whitespace. The administrators were concerned that if they inadvertently put whitespace at the end of the matcher, it would not be apparent from the management screen what was causing the mismatch.

I agreed and left the meeting with the intention of making the matcher values case-insensitive and leading/trailing whitespace agnostic.

I returned to the code to make the change.

Back in the code, I realized that I had inadvertently created a difficult situation for myself. Earlier, I had decided that the matchers should own all of the logic with regards to matching the data. They would be given a data from a field, a type, and some matcher values. They were expected to use this information to generate a match distance. This left me in an awkward situation. If I wanted to change how a text condition was altered, I would have to adjust every matcher that was capable of matching text (most of them). Major bummer.

I made the change to every relevant matcher.

During a code review with one of my esteemed colleagues, he noticed that I had changed all of the matchers. He pointed out that something was probably wrong structurally, and I conceded the point.

While I was distracted with design decisions

“How do I layout this data?”

“Is sequence important?”

“Which matchers should I support?”

“Which fields should I support?”

“How will I handle typed matching?”

“Is this screen easy to use?”

I had walked directly into a poor design outcome. We began pair programming. After analyzing the situation, we determined that the type coercion should belong to the type. After moving the coercion into the type, it became apparent that the comparison logic also belonged to the type. After an afternoon of extraction and restructuring, the matchers only contained a small amount of logic with regard to how the comparison logic provided by a type should be used and which distance should result from the comparison. The code became much simpler and easier to reason about. The change will likely reduce the cost of future changes (such as adding more matchers)

Why was I comfortable Making the Change?

I was comfortable making the change for three reasons

1. The code was heavily tested. I was not worried about damaging the functionality
2. I had a second set of eyes keeping me honest and helping me see solutions that I would not have seen alone
3. I could that we were working towards a better solution

Retrospective

I’m going to take a moment here to address the SOLID principles that I later realized I had violated

S - Single Responsibility

By putting the type coercion into the matchers, I had given the matchers more responsibility than they should have had. I justified this to myself at the time by thinking that the type belonged to the comparison. The request for change later showed that this assumption was incorrect. I had in-fact broken the single responsibility twice. The matchers knew how to turn a generic type into a specific type and they also knew how to compare the specific type. It’s a glaring violation, looking backwards, but I was lost in solving the “big picture” and I was blind to the poor decision in front of me.

O - Open Closed Principle

All of the matchers should have been open to extension and closed for modification. To sanitize the text going into the matcher, I should have only had to add a layer to transform the text before it was sent into the matcher. Because I was violating Single Responsibility by claiming ownership of the type within the matcher, the structure demanded that I adjust each matcher so that the text would be handled appropriately.

Violation rating: (S2O)

3 violations. Oof. My bones.

Learning from mistakes is not always fun. It’s embarrassing to make mistakes around others. I had the good fortune to be able to clean up the mess that I had made. If this problem hadn’t come up during a code review, I may well have just changed the matchers and moved on. The change was inconvenient to make, but I had made the decision earlier and it is difficult to see a problem from a scope that has already been chosen. This makes code reviews and pair programming important. The code review told me that something was wrong. I paired with the colleague that noticed the problem so that I could grow as a developer. Software development is a humility game. Good code and proud developers do not often go together. I cleaned up my mess with an audience, and it didn’t make me feel proud, but the code was healthier afterwards and I had gained experience.