Tests alone are not enough, good architecture is needed

    We all understand what automated tests are. We are developing software, and we want it to solve some user problems. By writing a test, we make sure that a specific problem is solved by a specific section of code. Then the requirements change, we change the tests and change the code according to the new requirements. But this does not always save. In addition to high test coverage, our code should be designed in such a way as to protect the developer from errors while writing it.

    In the article, I tried to describe one of the problems that a good architecture can solve: related sections of code can disperse among themselves, this can lead to bugs, and tests will not save here. A competent design can help.

    Life example


    We are developing a marketing CRM system for aggregating customer data from online stores. It is possible to load orders into this platform using the API, and then analyze which consumers buy what, what can be recommended, and the like. You also need to be able to see the change history for each order in the interface.

    First, imagine our domain model.

    We have buyers, orders and order items. A buyer can have many orders, an order can have many lines. Each line contains the base unit price, line price, quantity, product identifier, a link to the order in which the line is contained, as well as the line status (for example, “issued” or “canceled”). The order contains the customer’s order identifier, the date and time of the order, the cost of the order, taking into account all the discounts, shipping costs, a link to the store of purchase, as well as a link to the consumer.
    We also have a requirement to view the history of order changes and a ban on deleting data about orders that were once downloaded to us. As a result, the following class structure is obtained:

    Customer
    classCustomer
    {
    	publicint Id { get; set; }
    	publicstring Name { get; set; }
    	publicstring Email { get; set; }
    }


    Order
    classOrder 
    {
    	publicint Id {get;set;}
    	publicstring ExternalId {get;set;}
    	public ICollection<OrderHistoryItem> History {get;}
    }


    Historical status of the order
    classOrderHistoryItem 
    {
    	public ICollection<OrderHistoryLine> OrderLines {get;}
    	public Order Order { get; set; }
    	public DateTime OrderChangeDateTime { get; set; }
    	publicdecimal TotalAmountAfterDiscounts { get; set; }
    	public Customer Customer { get; set; }
    	publicdecimal DeliveryCost { get; set; }
    	publicstring ShopExternalId { get; set; }
    }


    Order item
    classOrderHistoryLine
    {
    	publicdecimal BasePricePerItem { get; set; }
    	public OrderHistoryItem OrderHistoryItem { get; set; }
    	publicdecimal PriceOfLine { get; set; }
    	publicdecimal Quantity { get; set; }
    	publicstring Status { get; set; }
    	publicstring ProductExternalId { get; set; }
    }


    Order is an aggregate that can contain many historical order states (OrderHistoryItem), each of which contains lines (OrderHistoryLine). Thus, when changing the order, we will save its state before and after the change. At the same time, the current state of the order can always be obtained by ordering historical conditions by time and taking the first one.

    Now we describe the format of the API request that our customers could use to transfer orders:

    API format
    {
    	id: “123”,
    	customer: {  id: 1 },
    	dateTimeUtc: “21.08.201711:11:11”,
    	totalAmountAfterDiscounts: 2000,
    	shop: “shop-moscow”,
    	deliveryCost: 0,
    	items: [{
    		basePricePerItem: 100,
    		productId: “456”,
    		status: “Paid”,
    		quantity: 20,
    		priceOfLineAfterDiscounts: 2000	
    	}]
    }


    Imagine that we implemented this API and wrote tests on it. Our customers started using it, and we realized the problem.

    The fact is that our API implies a fairly simple integration on the client side (for example, an online store). It is assumed that the order data transfer service should be called by the client for each change of the order on its side. Thus, the complexity of integration is reduced, since there is no logic other than broadcasting on the client side.
    However, it happens that our system is not indifferent to some changes in orders that occur in the client system, since they are of no interest to marketing and analytics. Such changes to the order on our side simply will not be any different.

    In addition, there may be situations when, due to some network errors, the client uses our service several times for the same order change, and we save them all. To defeat network plan errors when the same transaction is transmitted several times, you can use the idempotency key, but it will not help to solve the problem of minor changes. We really do not want to store several changes to the order for the business, if they are no different.

    No sooner said than done. A solution to the forehead: let's write a code that, when a new change to an order arrives, will look for exactly the same among existing changes in the same order, and if it is, it simply won’t save another one. The code will look something like this (unless you think about optimizations with hash calculation):

    publicvoidSaveOrder(OrderHistoryItem historyItem) {
    	if (historyItem.Order.IsNew)
    	{
    		SaveOrderCore(historyItem.Order);
    		SaveOrderHistoryCore(historyItem);
    		return;
    	}
    	var doesSameHistoryItemExist = historyItem.Order.History
    		.Where(h => h != historyItem)
    		.Where(h => h.IsEquivalent(historyItem))
    		.Any();
    	if (doesSameHistoryItemExist)
    		return;
    	SaveOrderHistoryCore(historyItem);
    }

    In this code, we go through all the historical records of the order and look for the same using the IsEquivalent method. In this case, IsEquivalent must compare each order field, and then look for the corresponding order lines and compare each field of these lines. A naive implementation might look like this:

    publicboolIsEquivalent(OrderHistoryItem otherHistoryItem)
    {
    	return IsTotalPriceEquivalent(otherHistoryItem) &&
    		IsDeliveryCostEquivalent(otherHistoryItem) &&
    		IsShopEquivalent(otherHistoryItem) &&
    		AreLinesEquivalent(otherHistoryItem);
    }

    Tests do not save


    OrderHistoryItem.IsEquivalent is a pretty simple function, it's pretty easy to test. By writing tests on it, we will make sure that the business requirement that prohibits the creation of several identical states of the same order is met.

    However, there is a problem here, which is that in enterprise systems, like the one we just talked about, quite a lot of code. Our company has 5 teams of 5 developers, we all develop the same product, and already at such a scale it is quite difficult to organize work so that each person in each team understands well the subject area of ​​each feature of the product. In principle, this is not really necessary, since teams are divided by features, but sometimes features intersect, and then difficulties arise. The problem I'm talking about is the complexity of a large system where there are many components that need to be connected.

    Imagine that we wanted to add another field to the order: the type of payment.
    Fix the OrderHistoryItem class:

    class OrderHistoryItem with the added PaymentType property
    classOrderHistoryItem 
    {
    	public Order Order { get; set; }
    	public DateTime OrderChangeDateTime { get; set; }
    	publicdecimal TotalAmountAfterDiscounts { get; set; }
    	public Customer Customer { get; set; }
    	publicdecimal DeliveryCost { get; set; }
    	publicstring ShopExternalId { get; set; }
    	publicstring PaymentType {get;set;}
    }
    


    In addition, we must add the ability to transfer the type of payment to our service.

    Have we forgotten anything? Oh yes, we need to fix IsEquivalent. And this is very sad, since the feature “not to create duplicates of the same order changes” is quite invisible, it is difficult to remember. Will the product owner write how we should consider the type of payment when comparing orders? Will the architect remember this feature? Will the programmer who will implement the type of payment and add it to the service think about this? Indeed, if no one remembers this, then no test will fall , and in a situation where we will receive an order change first with one type of payment, and then with another, the second change will not be saved.

    One of the most important quality criteria for the selected application architecture is the simplicity of modifying the application to the changing business requirements. Also one of the pillars of the development of non-trivial software is the reduction of complexity. The problem that I described above is connected with high complexity and the possibility of changes. In short, the architecture should be one in which it would be impossible to forget that you need to somehow compare each of the properties of the history of order changes.

    Declarative strategies as a way out


    We wrote an imperative code to verify the properties of an order, which is easy to test but difficult to analyze. Let's try to take advantage of the declarative approach. Now the IsEquivalent method internally calls the IsTotalPriceEquivalent, IsDeliveryCostEquivalent and the like methods. We introduce the interface of the strategy for comparing the order property IOrderHistoryItemPropertyEqualityStrategy:

    interfaceIOrderHistoryItemPropertyEqualityStrategy 
    {
    	boolArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem);
    }

    Let's do the implementation instead of each comparison method in IsEquivalent:

    classTotalPriceOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy 
    {
    	publicboolArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem){
    		return firstOrderHistoryItem.TotalAmountAfterDiscounts == secondOrderHistoryItem.TotalAmountAfterDiscounts;
    	}
    }

    classDeliveryCostOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy
    {
    	publicboolArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem)
    	{
    		return firstOrderHistoryItem.DeliveryCost == secondOrderHistoryItem.DeliveryCost;
    	}
    }

    and make the comparison more declarative:

    publicboolIsEquivalent(OrderHistoryItem otherHistoryItem)
    {
    	returnnew TotalPriceOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem) &&
    		new DeliveryCostOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem) &&
    		new ShopOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem) &&
    		new LinesOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem);
    }

    A little refactory:

    privatestatic IOrderHistoryItemPropertyEqualityStrategy[] equalityStrategies = new[] {
    	new TotalPriceOrderHistoryItemPropertyEqualityStrategy(),
    	new DeliveryCostOrderHistoryItemPropertyEqualityStrategy(),
    	new ShopOrderHistoryItemPropertyEqualityStrategy(),
    	new LinesOrderHistoryItemPropertyEqualityStrategy()
    }
    publicboolIsEquivalent(OrderHistoryItem otherHistoryItem)
    {
    	return equalityStrategies.All(strategy => strategy.ArePropertiesEquivalent(this, otherHistoryItem))
    }

    Now when adding a new property, you need to create a new property comparison strategy and add it to the array. While nothing has changed, we are still not protected from error. But due to the fact that the code has become declarative, we can check something. It would be great to be able to understand which property is responsible for which property. Then we could get all the properties from the order and verify that they are all covered by strategies. And it's pretty easy to refine. Add the PropertyName property to the strategy interface and implement it in each of the strategies:

    publicinterfaceIOrderHistoryItemPropertyEqualityStrategy 
    {
    	string PropertyName {get;}
    	boolArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem);
    }
    publicclassTotalPriceOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy 
    {
    	publicstring PropertyName => nameof(OrderHistoryItem.TotalAmountAfterDiscounts);
    	publicboolArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem){
    		return firstOrderHistoryItem.TotalAmountAfterDiscounts == secondOrderHistoryItem.TotalAmountAfterDiscounts;
    	}
    }

    Now write a check:

    publicstaticvoidCheckOrderHistoryEqualityStrategies() {
    	var historyItemPropertyNames = typeof(OrderHistoryItem)
    		.GetProperties()
    		.Select(p => p.Name);
    	var equalityStrategiesPropertyNames = equalityStrategies
    		.Select(es => es.PropertyName);
    	var propertiesWithoutCorrespondingStrategy = historyItemPropertyNames
    		.Except(equalityStrategiesPropertyNames)
    		.ToArray();
    	if (!propertiesWithoutCorrespondingStrategy.Any())
    		return;
    	var missingPropertiesString = string.Join(", ", propertiesWithoutCorrespondingStrategy);
    	thrownew InvalidOperationException($"Для свойств {missingPropertiesString} не зарегистрировано стратегии сравнения свойства");
    }

    Now it’s enough to write a test for this test, and you can be calm that we will forget something. Separately, I note that the implementation shown is not complete. Firstly, properties that do not need to be compared are not taken into account. For them, you can use this strategy implementation:

    publicclassNoCheckOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy
    {
    	privatestring propertyName;
    	publicNoCheckOrderHistoryItemPropertyEqualityStrategy(string propertyName) {
    		this.propertyName = propertyName;
    	}
    	publicstring PropertyName => propertyName;
    	publicboolArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem)
    	{
    		returntrue;
    	}
    }

    Secondly, it is not shown how to make such a strategy for complex properties, such as lines. Since this is done in exactly the same way, you can think of it yourself.

    In addition, a commitment to defensive programming may suggest that it is worth checking that more than one strategy for each property has not been registered, and that strategies for unknown properties have not been registered. But these are the details.

    Other examples


    Consumer Removal


    Another example of a similar problem we had was the “consumer removal” functionality. The problem with deleting was that quite a lot of things were connected with the consumer in our system, and when deleting, you need to think about all the entities with which consumers are connected, possibly indirectly through other entities, and this functionality should have been polymorphic, because end customers may have more entities than the main product.

    As you can see, the problem is the same: if you add a new entity or even just some kind of link to forget that there is a deletion, you can get a bug in the production of FK violation when trying to delete.

    A similar solution is proposed: it is quite possible to bypass the entire data model by means of a relatively complex reflexive code, including in final implementations for a particular client, at application startup. In this model, find which entities are connected in a way with consumers. After that, look if removal strategies are registered for all of these entities, and if something is missing, just drop.

    Filter Time Recalculation Providers


    Another example of the same problem: registering filter conditions. We have a rather flexible filter constructor in the UI:



    These filters are used both for filtering on the pages of lists of almost any entities, as well as for setting up any automatically triggered mechanics - triggers.
    The trigger works like this: every 15 minutes it selects all the consumers that it needs to work on, just using the complex filtering condition collected in the designer, and processes them in turn, sending them a letter, scoring points or giving out some kind of prize or discount.

    Over time, it became clear that this approach needs to be optimized, because there are many triggers, there are many consumers, and it is difficult to choose them all every 15 minutes. Optimization, which suggested itself - let's not check consumers who have not changed, each time we will look only at changed consumers. This would be a completely valid approach, if not one but: some filtering conditions become true for the consumer, not because something happened to him, but because the time has simply come. Such conditions are all sorts of temporary conditions, such as “less than 5 days left before the birthday”.

    We have identified all the filtering conditions, the truth of which may change from time to time, and decided that they should tell us the time for which it makes sense to count them. Roughly speaking, if we count in days, then it makes sense to count every 12 hours, if in hours - then every hour.

    The problem is that the module in which our filtering conditions are defined is one of the most basic, and we would not want it to think about what the conversion time should be. Therefore, the interface for obtaining the conversion time for the filtering condition is separate from the filtering conditions themselves, and we again find ourselves in a situation where when creating a new filtering condition we can easily forget that it requires specifying the conversion time.

    The solution is the same as before. We have an application initialization phase when filtering conditions are added. After that, there is a stage when conversion time providers are added for filtering conditions in another module. After that, it is critical to verify that for all filtering conditions, such providers are registered. In no case can you rely on the default behavior: if the provider for obtaining time is not registered, assume that the filter does not require recounting. This feature is very tempting, because refactoring in such a situation looks much simpler: you need to register providers only for those filtering conditions that have been selected. If you choose this approach, you can save time now, but then someone will write a new filtering condition by time, forget about

    Finally


    That's all. I tried to explain my vision of the “unwritten code” problem in the article. Unfortunately, we cannot verify the absence of any code in this way, we need to push off from something, look for some inconsistencies in the configuration of a large system, and fall, fall, fall, as long as they exist.

    Try to make the architecture impenetrable, so that when used improperly, it always swears, so you can protect yourself from unpleasant mistakes in battle. Try not to compromise with reliability: if you need to refactor a lot of code to reduce risks in the future, then you should still spend time.

    Also popular now: