Why Abstraction is Really Important

Abstraction
Abstraction is one of the key elements of good software design.
It helps encapsulate behavior. It helps decouple software elements. It helps having more self-contained modules. And much more.

Abstraction makes the application extendable in much easier way. It makes refactoring much easier.
When developing with higher level of abstraction, you communicate the behavior and less the implementation.

General
In this post, I want to introduce a simple scenario that shows how, by choosing a simple solution, we can get into a situation of hard coupling and rigid design.

Then I will briefly describe how we can avoid situation like this.

Case study description
Let’s assume that we have a domain object called RawItem.

public class RawItem {
    private final String originator;
    private final String department;
    private final String division;
    private final Object[] moreParameters;
    
    public RawItem(String originator, String department, String division, Object... moreParameters) {
        this.originator = originator;
        this.department = department;
        this.division = division;
        this.moreParameters = moreParameters;
    }
}

The three first parameters represent the item’s key.
I.e. An item comes from an originator, a department and a division.
The “moreParameters” is just to emphasize the item has more parameters.

This triplet has two basic usages:
1. As key to store in the DB
2. As key in maps (key to RawItem)

Storing in DB based on the key
The DB tables are sharded in order to evenly distribute the items.
Sharding is done by a hash key modulo function.
This function works on a string.

Suppose we have N shards tables: (RAW_ITEM_REPOSITORY_00, RAW_ITEM_REPOSITORY_01,..,RAW_ITEM_REPOSITORY_NN),
then we’ll distribute the items based on some function and modulo:

String rawKey = originator + "_"  + department + "_" + division;
// func is String -> Integer function, N = # of shards
// Representation of the key is described below
int shard = func(key)%N;

Using the key in maps
The second usage for the triplet is mapping the items for fast lookup.
So, when NOT using abstraction, the maps will usually look like:

Map<String, RawItem> mapOfItems = new HashMap<>();
// Fill the map...

“Improving” the class
We see that we have common usage for the key as string, so we decide to put the string representation in the RawItem.

// new member
private final String key;

// in the constructor:
this.key = this.originator + "_" + this.department + "_"  + this.division;

// and a getter
public String getKey() {
  return key;
}

Assessment of the design
There are two flows here:
1. Coupling between the sharding distribution and the items’ mapping
2. The mapping key is strict. any change forces change in the key, which might introduce hard to find bugs

And then comes a new requirement
Up until now, the triplet: originator, department and division made up a key of an item.
But now, a new requirement comes in.
A division can have subdivision.
It means that, unlike before, we can have two different items from the same triplet. The items will differ by the subdivision attribute.

Difficult to change
Regarding the DB distribution, we’ll need to keep the concatenated key of the triplet.
We must keep the modulo function the same. So distribution will remain using the triplets, but the schema will change and hava ‘subdivision’ column as well.
We’ll change the queries to use the subdivision together with original key.

In regard to the mapping, we’ll need to do a massive refactoring and to pass an ItemKey (see below) instead of just String.

Abstraction of the key
Let’s create ItemKey

public class ItemKey {
    private final String originator;
    private final String department;
    private final String division;
    private final String subdivision;

    public ItemKey(String originator, String department, String division, String subdivision) {
        this.originator = originator;
        this.department = department;
        this.division = division;
        this.subdivision = subdivision;
    }

    public String asDistribution() {
        return this.originator + "_" + this.department + "_"  + this.division;
    }
}

And,

Map<ItemKey, RawItem> mapOfItems = new HashMap<>();
// Fill the map...
    // new constructor for RawItem
    public RawItem(ItemKey itemKey, Object... moreParameters) {
        // fill the fields
    }

Lesson Learned and conclusion
I wanted to show how a simple decision can really hurt.

And, how, by a small change, we made the key abstract.
In the future the key can have even more fields, but we’ll need to change only the inner implementation of it.
The logic and mapping usage should not be changed.

Regarding the change process,
I haven’t described how to do the refactoring, as it really depends on how the code looks like and how much is it tested.
In our case, some parts were easy, while others were really hard. The hard parts were around code that was looking deep in the implementation of the key (string) and the item.

This situation was real
We actually had this flow in our design.
Everything was fine for two years, until we had to change the key (add the subdivision).
Luckily all of our code is tested so we could see what breaks and fix it.
But it was painful.

There are two abstraction that we could have initially implement:
1. The more obvious is using a KEY class (as describe above). Even if it only has one String field
2. Any map usage need to be examined whether we’ll benefit by hiding it using abstraction

The second abstraction is harder to grasp and to fully understand and implement.

So,
do abstraction, tell a story and use the interfaces and don’t get into details while telling it.

Linkedin Twitter facebook github

Advertisements

The Foreman Role in a Team

There is a lot of discussion about the need for a foreman role in a software team.
Robert C. Martin wrote about it in Where is the Foreman?

I recently read a post by David Tanzer who disagrees with Uncle Bob’s point: We don’t need a foreman

The way I see it, a foreman role is important, but perhaps not as extreme as Uncle Bob describes.
Let’s start by quoting the foreman’s role as Uncle Bob describes (in construction and in software):

The foreman on a construction site is the guy who is responsible for making sure all the workers do things right. He’s the guy with the tape-measure that goes around making sure all the walls are placed properly. He’s the guy who examines all the struts, joists, and beams to make sure they are installed correctly, and don’t have any significant defects. He’s the guy who counts the screws in the flooring to make sure it won’t squeak when you walk on it. He’s the guy — the guy who takes responsibility — the guy who makes sure everything is doneĀ right.

What would the foreman do on software project? He’d do the same thing he does on a construction project. He’d make sure everything was done, done right, and done on time. He’d be the only one with commit rights. Everybody else would send him pull requests. He’d review each request in turn and reject those that didn’t have sufficient test coverage, or that had dirty code, or bad variable names, or functions that were too long. He’d reject those that, in his opinion, did not meet the level of quality he demands for the project.

I think that the commit rights is crucial point for many critics of this idea.

I would like to suggest a middle way.
A foreman role, but also team’s responsibility.

In real life the team is diverse. Some are seniors, some juniors. Some are expert in a specific field, others in another field. Some are expert in TDD, some in design, some in DB and SQL etc.

Here are the key points as I see it:
A diverse team.
A foreman who’s responsible for the quality and delivery.
The foreman will sometimes make the final call. Even if there is still a disagreement.
The foreman is also responsible that everyone works at the standards he introduced (with the help of the team).
Everyone can commit. Not just the foreman.
Everyone can suggest anything.

Here are some reasons why it might work.

  1. Everyone can commit and everyone can see others’ commits. This means that there is trust between the team members. It also gives each member more responsibility. The foreman in this case will still look for all the things that Uncle Bob describes. But when he sees something wrong (missing test? A code that is not well designed?) then he will approach the person who committed the code and discuss what went wrong. The foreman will have an opportunity to mentor other team members and pass his knowledge.
  2. The foreman can be the peer, with more responsibilities. If Fred notices that people make mistakes, he will discuss it. The foreman has more responsibility. He needs to know to listen. He needs to explain and not blame.
  3. The foreman does not have to be the most experienced developer in everything. He can’t be. He may be most experienced in one or two or three fields. But not all. So if Alice is the most experienced DB developer, Fred the foreman should see that she helps other team members with SQL related stuff. He will still remind Alice about the procedures and code of the whole system.
  4. Sometimes the foreman will need to make decisions. Sometimes not everyone will agree. The foreman needs to know when to stop an argument and give the call.
  5. The foreman doesn’t need to have the sole responsibility for quality. But he’s the one that the management should approach. This is a tricky part. It’s hard to achieve. The team is responsible for the quality and delivery of the code. The foreman is responsible that the team achieve this. The foreman is responsible that everyone practices good coding (and everything that implies). The foreman is the one who needs to mentor team members who do not know how to bring quality code.
  6. The team is responsible for the architecture and design. As I mentioned before, the foreman will sometimes need to stop the discussion and make a decision. Each member of the team should have the opportunity to come forward with suggestions. Sometimes the foreman will bring the best ideas (after all, he’s supposed to be the most experienced), but more than once, other member will introduce the correct design. The foreman needs to listen.
  7. During the planning the team will estimate effort (E.g. will give points to user stories). Then, if the whole team is responsible for the design and architecture, the members will create the tasks with some time estimations. The foreman’s responsibility would be to see that everyone understands the priorities. He should be part of the team while designing and lead the design. If the team did not understand the priorities and didn’t bring quality code, it’s his responsibility. But also the team’s.
  8. The foreman should introduce new technologies to the team. The foreman should introduce the team coding practices. The foreman must pair with other members. Juniors and seniors. While pairing with juniors he actually mentors them. The foreman must see that the team does pair-programming with each other. The foreman is the one that establish code review habits in the team. As a foreman he can ask to review code even it was already done by another person. Sometimes it brings some antagonism, but as mentioned before, he has the responsibility and he’s the one that needs to answer the management.

Uncle Bob suggested a rather extreme approach.
Perhaps it suits in some extreme cases. He describes an open source project that actually work with several foremen: A Spectrum of Trust
On the other side, David Tanzer shows correctly why this approach may deteriorate the team spirit and trust.

I think that it’s possible to have a middle way.
I think that a team can have a foreman, a person who’s in charge. But still let everyone be involved. Have trust, spirit and motivation.

Linkedin Twitter facebook github

Agile Mindset During Programming

I’m Stuck

Recently I found myself in several situations where I just couldn’t write code. Or at least, “good code”
First, I had “writer’s block”. I just could not see what was going to be my next test to write.
I could not find the name for the class / interface I needed.
Second, I just couldn’t simplify my code. Each time I tried to change something (class / method) to a simpler construction, things got worse. Sometimes to break.

I was stuck.

The Tasks

Refactor to Patterns

One of the situation we had was to refactor a certain piece in the code.
This piece of code is the manual wiring part. We use DI pattern in ALL of our system, but due to some technical constraints, we must do the injection by hand. We can live with that.
So the refactor in the wiring part would have given us a nice option to change some of the implementation during boot.
Some of the concrete classes should be different than others based on some flags.
The design patterns we understood we would need were: Factory Method and Abstract Factory
The last remark is important to understand why I had those difficulties.
I will get to it later.

New Module

Another task was to create a new module that gets some input items, extract data from them, send it to a service, parse the response, modify the data accordingly and returns items with modified data.
While talking about it with a peer, we understood we needed several classes.
As always we wanted to have high quality code by using the known OOD principles wherever we could apply them.

So What Went Wrong?

In the case of refactoring the wiring part, I constantly tried to immediately create the end result of the abstract factory and the factory method that would call it.
There are a-lot of details in that wiring code. Some are common and some needed to be separated by the factory.
I just couldn’t find the correct places to extract to methods and then to other class.
Each time I had to move code from one location and dependency to another.
I couldn’t tell what exactly the factory’s signature and methods would be.

In the case of the new module, I knew that I want several classes. Each has one responsibility. I knew I want some level of abstraction and good encapsulation.
So I kept trying to create this great encapsulated abstract data structure. And the code kept being extremely complicated.
Important note: I always to test first approach.
Each time I tried to create a test for a certain behavior, it was really really complicated.

I stopped

Went to have a cup of coffey.
I went to read some unrelated stuff.
And I talked to one of my peers.
We both understood what we needed to do.
I went home…

And then it hit me

The problem I had was that I knew were I needed to go, but instead of taking small steps, I kept trying to take one big leap at once.
Which brings me to the analogy of Agile to good programming habits (and TDD would be one of them).

Agile and Programming Analogy

One of the advantages in Agile development that I really like is the small steps (iteration) we do in order to reach our goal.
Check the two pictures below.
One shows how we aim towards a far away goal and probably miss.
The other shows how we divide to iterations and aim incrementally.

Aiming From Far

Aiming From Far


Aiming Iterative and Incremental

Aiming Iterative and Incremental

Develop in Small Incremental Iterations

This is the moral of the story.
Even if you know exactly how the structure of the classes should look like.
Even if you know exactly which design pattern to use.
Even if you know what to do.
Even if you know exactly how the end result should look like.

Keep on using the methods and practices that brings you to the goal in the safest and fastest way.
Do small steps.
Test each step.
Increment the functionality of the code in small chucks.
TDD.
Pair.
Keep calm.

Refactor Big Leap

Refactor Big Leap


Refactor Small Steps

Refactor Small Steps


Request Validation and Filtering by Flags – Redesign and Refactoring

General
In the previous posts I started describing a validation / filtering framework we’re building.
While showing the code, I am trying to show clean code, test orientation and code evolution.
It has some agility in the process; We know the end requirements, but the exact details are evolving over time.

During the development we have changed the code to be more general as we saw some patterns in it.
The code evolved as the flow evolved as well.

The flow as we now understand it
Here’s a diagram of the flow we’ll implement

Request Sequence

Request Sequence

The Pattern
At each step of the sequence (validation, filtering, action), we recognized the same pattern:

  1. We have specific implementations (filters, validations)
  2. We have an engine that wraps up the specific implementations
  3. We need to map the implementations by flag, and upon request’s flags, select the appropriate implementations.
  4. We need to have a class that calls the mapper and then the engine

A diagram showing the pattern

The Pattern

The Pattern

Source Code
In order to show some of the evolution of the code, and how refactoring changed it, I added tags in GitHub after major changes.

Code Examples
Let’s see what came up from the mapper pattern.

public interface MapperByFlag<T> {
  List<T> getOperations(Request request);
}
public abstract class AbstractMapperByFlag<T> implements MapperByFlag<T> {
  private List<T> defaultOperations;
  private Map<String, List<T>> mapOfOperations;

  public AbstractMapperByFlag(List<T> defaultOperations, Map<String, List<T>> mapOfOperations) {
    this.defaultOperations = defaultOperations;
    this.mapOfOperations = mapOfOperations;
  }

  @Override
  public final List<T> getOperations(Request request) {
    Set<T> selectedFilters = Sets.newHashSet(defaultOperations);
    Set<String> flags = request.getFlags();
    for (String flag : flags) {
      if (mapOfOperations.containsKey(flag)) {
        selectedFilters.addAll(mapOfOperations.get(flag));
      }
    }
    return Lists.newArrayList(selectedFilters);
  }
}
  public RequestValidationByFlagMapper(List<RequestValidation> defaultValidations,
    map<String, List<RequestValidation>> mapOfValidations) {
    super(defaultValidations, mapOfValidations);
  }

  public ItemFiltersByFlagMapper(List<Filter> defaultFilters, Map<String, List<Filter>> mapOfFilters) {
    super(defaultFilters, mapOfFilters);
  }

I created a test for the abstract class, to show the flow itself.
The tests of the implementations use Java Reflection to verify that the correct injected parameters are sent to the super.
I am showing the imports here as well. To have some reference for the static imports, mockito and hamcrest packages and classes.

import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.junit.Assert.assertThat;
import static org.mockito.Mockito.when;

import java.util.List;
import java.util.Map;

import org.eyal.requestvalidation.model.Request;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;

@RunWith(MockitoJUnitRunner.class)
public class AbstractMapperByFlagTest {
	private final static String FLAG_1 = "flag 1";
	private final static String FLAG_2 = "flag 2";

	@Mock
	private Request request;

	private String defaultOperation1 = "defaultOperation1";
	private String defaultOperation2 = "defaultOperation2";
	private String mapOperation11 = "mapOperation11";
	private String mapOperation12 = "mapOperation12";
	private String mapOperation23 = "mapOperation23";

	private MapperByFlag<String> mapper;

	@Before
	public void setup() {
		List<String> defaults = Lists.newArrayList(defaultOperation1, defaultOperation2);
		Map<String, List<String>> mapped = ImmutableMap.<String, List<String>> builder()
		        .put(FLAG_1, Lists.newArrayList(mapOperation11, mapOperation12))
		        .put(FLAG_2, Lists.newArrayList(mapOperation23, mapOperation11)).build();
		mapper = new AbstractMapperByFlag<String>(defaults, mapped) {
		};
	}

	@Test
	public void whenRequestDoesNotHaveFlagsShouldReturnDefaultFiltersOnly() {
		when(request.getFlags()).thenReturn(Sets.<String> newHashSet());

		List<String> filters = mapper.getOperations(request);
		assertThat(filters, containsInAnyOrder(defaultOperation1, defaultOperation2));
	}

	@Test
	public void whenRequestHasFlagsNotInMappingShouldReturnDefaultFiltersOnly() {
		when(request.getFlags()).thenReturn(Sets.<String> newHashSet("un-mapped-flag"));
		List<String> filters = mapper.getOperations(request);
		assertThat(filters, containsInAnyOrder(defaultOperation1, defaultOperation2));
	}
	
	@Test
	public void whenRequestHasOneFlagShouldReturnWithDefaultAndMappedFilters() {
		when(request.getFlags()).thenReturn(Sets.<String> newHashSet(FLAG_1));
		List<String> filters = mapper.getOperations(request);
		assertThat(filters, containsInAnyOrder(mapOperation12, defaultOperation1, mapOperation11, defaultOperation2));
	}
	
	@Test
	public void whenRequestHasTwoFlagsShouldReturnWithDefaultAndMappedFiltersWithoutDuplications() {
		when(request.getFlags()).thenReturn(Sets.<String> newHashSet(FLAG_1, FLAG_2));
		List<String> filters = mapper.getOperations(request);
		assertThat(filters, containsInAnyOrder(mapOperation12, defaultOperation1, mapOperation11, defaultOperation2, mapOperation23));
	}
}
@RunWith(MockitoJUnitRunner.class)
public class RequestValidationByFlagMapperTest {

	@Mock
	private List<RequestValidation> defaultValidations;
    
	@Mock
	private Map<String, List<RequestValidation>> mapOfValidations;

	@InjectMocks
	private RequestValidationByFlagMapper mapper;

	@SuppressWarnings("unchecked")
    @Test
	public void verifyParameters() throws NoSuchFieldException, SecurityException, IllegalArgumentException,
	        IllegalAccessException {
		Field defaultOperationsField = AbstractMapperByFlag.class.getDeclaredField("defaultOperations");
		defaultOperationsField.setAccessible(true);
        List<RequestValidation> actualFilters = (List<RequestValidation>) defaultOperationsField.get(mapper);
		assertThat(actualFilters, sameInstance(defaultValidations));

		Field mapOfFiltersField = AbstractMapperByFlag.class.getDeclaredField("mapOfOperations");
		mapOfFiltersField.setAccessible(true);
		Map<String, List<RequestValidation>> actualMapOfFilters = (Map<String, List<RequestValidation>>) mapOfFiltersField.get(mapper);
		assertThat(actualMapOfFilters, sameInstance(mapOfValidations));
	}
}

To Do
There are other classes that might be candidate for refactoring of some sort.
RequestFlowValidation and RequestFilter are similar.
And
RequestValidationsEngineImpl and FiltersEngine

To Do 2
Create a Matcher for the reflection part.

Code
As always, all the code can be found at:

A Tag for this post: all-components-in

Conclusion
The infrastructure is almost done.
During this time we are also implementing actual classes for the flow (validations, filters, actions).
These are not covered in the posts, nor in GitHub.
The infrastructure will be wired to a service we have using Spring.
This will be explained in future posts.