Skip to content
wannacode

Architecture review

In short: An architecture review reads the real dependencies from the code, finds arrows pointing the wrong way and cycles, and turns the rules into a fitness function that guards them on every commit.

Problem

The diagram says "layers"

You inherit the online shop. The README has a diagram: four layers, arrows only downwards.

README.md
web      web/shop
service  service/orders  service/payments
domain   domain/pricing
data     data/orders


web → service → domain → data

Your first task is small: the price on the page should have two decimal places. You change formatPrice in web/shop, and the payment tests fail. It turns out that domain/pricing uses formatPrice from web/shop, and orders and payments call each other.

The diagram did not lie on purpose. It was true two years ago, and then nobody checked it.

The idea

The truth is in the imports

An architecture review answers a few questions about a system that already runs:

  1. What are the real boundaries? Which module calls which, according to the code.
  2. Are the rules followed? The layers from "Layers", the direction of dependencies from "Hexagonal", the module boundaries from "Monolith, modular monolith, services".
  3. Do the requirements hold? The percentiles and availability from "Non-functional requirements", measured on the live system, not promised.
  4. What can go wrong? The risks, ranked.
  5. Which decisions did nobody record? For those you write ADRs after the fact, so it is clear why the system is the way it is.

Here we start with the first two. Every import is an arrow from one module to another. A tool pulls them out of the code as a list, one dependency per line:

imports.txt
web/shop -> service/orders
service/orders -> domain/pricing
service/orders -> service/payments
service/payments -> service/orders
service/orders -> data/orders
domain/pricing -> web/shop

The program turns the list into a graph and looks for two things. An upward arrow: a lower layer calls a higher one. A cycle: a module that reaches itself again through other modules. A cycle means the two modules cannot be changed, tested or shipped separately. Cycles are found with a depth-first search with colours.

See it

Declared against real

static int Layer(string module) =>
    Array.IndexOf(Layers, module.Split('/')[0]);


// a lower layer calls a higher one: an upward arrow
static List<Dep> Violations(List<Dep> deps) =>
    deps.Where(d => Layer(d.From) > Layer(d.To))
        .ToList();


void Visit(string at)
{
    mark[at] = Mark.Grey; // grey: on the current path
    path.Add(at);
    foreach (var to in next[at])
    {
        if (!mark.TryGetValue(to, out var m)) Visit(to);
        else if (m == Mark.Grey) Cycle(to);
    }
    path.RemoveAt(path.Count - 1);
    mark[at] = Mark.Black; // black: done
}


void Cycle(string to)
{
    var loop = path.Skip(path.IndexOf(to)).Append(to);
    Found.Add(string.Join(" -> ", loop));
}
Step by step, in words
  1. The diagram in the README: four layers, arrows only downwards. Every module sits in its layer.
  2. The tool reads the imports: 6 real dependencies. Most point down or stay within a layer.
  3. domain/pricing imports web/shop: layer 2 calls layer 0. The arrow points upwards.
  4. A depth-first walk starts at web/shop. Every module on the current path is grey.
  5. domain/pricing points back to the grey web/shop: a cycle, web/shop → service/orders → domain/pricing → web/shop.
  6. The walk goes on: service/payments points back to the grey service/orders. A second cycle.
  7. Every module is done (black). Declared: layers, arrows only down. Real: 1 upward arrow and 2 cycles.

The layer rule is one comparison: the layer index grows downwards, so an upward arrow is layer(from) > layer(to).

static readonly string[] Layers = ["web", "service", "domain", "data"];


static int Layer(string module) =>
    Array.IndexOf(Layers, module.Split('/')[0]);


// a lower layer calls a higher one: an upward arrow
static List<Dep> Violations(List<Dep> deps) =>
    deps.Where(d => Layer(d.From) > Layer(d.To))
        .ToList();

A depth-first search finds the cycles. A grey module is on the current path, a black one is fully explored. An arrow to a grey module goes back along the path: that is a cycle, and the cycle is the part of the path from that module until now.

record Dep(string From, string To)
{
    public override string ToString() => $"{From} -> {To}";
}
enum Mark { Grey, Black }


class Cycles
{
    readonly Dictionary<string, List<string>> next = new();
    readonly Dictionary<string, Mark> mark = new();
    readonly List<string> path = new();
    public readonly List<string> Found = new();


    public Cycles(List<Dep> deps)
    {
        foreach (var d in deps)
        {
            next.TryAdd(d.From, new List<string>());
            next.TryAdd(d.To, new List<string>());
            next[d.From].Add(d.To);
        }
        foreach (var d in deps)
            if (!mark.ContainsKey(d.From)) Visit(d.From);
    }


    void Visit(string at)
    {
        mark[at] = Mark.Grey; // grey: on the current path
        path.Add(at);
        foreach (var to in next[at])
        {
            if (!mark.TryGetValue(to, out var m)) Visit(to);
            else if (m == Mark.Grey) Cycle(to);
        }
        path.RemoveAt(path.Count - 1);
        mark[at] = Mark.Black; // black: done
    }


    void Cycle(string to)
    {
        var loop = path.Skip(path.IndexOf(to)).Append(to);
        Found.Add(string.Join(" -> ", loop));
    }
}
TryGetValue also tells you whether the module is there. GetValueOrDefault would be a trap here: the default value of Mark is the first one, Grey, and every unvisited module would look grey.

The whole review on the list of imports:

var deps = Parse(Imports);
Console.WriteLine("Dependencies: " + deps.Count);
Show("Layer violations", Violations(deps));
Show("Cycles", new Cycles(deps).Found);
Output
Dependencies: 6
Layer violations:
  domain/pricing -> web/shop
Cycles:
  web/shop -> service/orders -> domain/pricing -> web/shop
  service/orders -> service/payments -> service/orders

The one violation and the two cycles are not three separate problems. domain/pricing -> web/shop is both the violation and the arrow that closes the first cycle: fix it, and both go away.

Try it

Fill in

Which comparison catches the upward arrow? The web layer has index 0 and data has index 3.

deps.Where(d => Layer(d.From)  Layer(d.To)).ToList();

Try it

Find the bug

A colleague "simplified" visit. Now the review prints:

Output
Dependencies: 6
Layer violations:
  domain/pricing -> web/shop
Cycles:
  none

Pick the line with the bug.

The idea

Fitness function: the review on every commit

A review is a snapshot. Tomorrow someone adds a new import and the snapshot is out of date. A fitness function is an automatic check of an architectural characteristic: it runs on every commit, like a test, and fails when the rule is broken. The name comes from the book "Building Evolutionary Architectures" (Ford, Parsons, Kua).

The inherited system already has a violation. If the check fails because of it, it will fail every day and the team will switch it off. So the old debt goes into a baseline: the check fails only for a new violation, and a fixed one must leave the baseline so it cannot come back unnoticed. This way the debt only shrinks.

// runs on every commit: the old debt is in the baseline
static List<string> Fitness(List<Dep> deps, List<string> baseline)
{
    var now = Violations(deps).Select(d => d.ToString()).ToList();
    var report = now.Where(v => !baseline.Contains(v))
        .Select(v => "✗ new upward dependency: " + v).ToList();
    report.AddRange(baseline.Where(old => !now.Contains(old))
        .Select(old => "✓ fixed, remove it from the baseline: " + old));
    return report;
}


static void Check(string commit, List<Dep> deps, List<string> baseline)
{
    var report = Fitness(deps, baseline);
    var fails = report.Any(r => r.StartsWith('✗'));
    Console.WriteLine(commit + ": " + (fails ? "FAILS" : "OK"));
    foreach (var r in report) Console.WriteLine("  " + r);
}

Commit 1 adds domain/pricing -> service/payments: pricing wants the payment fee. Commit 2 moves formatPrice into the domain and removes the old violation:

var deps = Parse(Imports);
List<string> baseline = ["domain/pricing -> web/shop"];


var first = deps.Append(new Dep("domain/pricing", "service/payments")).ToList();
Check("Commit 1", first, baseline);


var second = deps.Where(d => d.ToString() != "domain/pricing -> web/shop").ToList();
Check("Commit 2", second, baseline);
Output
Commit 1: FAILS
  ✗ new upward dependency: domain/pricing -> service/payments
Commit 2: OK
  ✓ fixed, remove it from the baseline: domain/pricing -> web/shop

A fitness function is not only for dependencies. The same idea guards other characteristics:

  • the p95 of payment in the load test stays under the budget from "Non-functional requirements";
  • the sellers module calls only the public API of orders, not its internals;
  • the page's JavaScript bundle is under 200 KB (an example limit);
  • every ADR has a "Consequences" section.

How exactly the check runs in the pipeline (CI) is a topic for ZeroToOps, the DevOps course. Here is the part the programmer writes: the check itself.

The idea

Risk register

The review finds more than there is time to fix. A risk register puts it in order. For each risk you record:

  • what can happen;
  • how likely it is, from 1 to 5;
  • how much it hurts if it happens (impact), from 1 to 5;
  • what you do about it and who owns it.

The score is probability × impact. You sort by it, and on a tie by impact: the rare but heavy event comes before the frequent, light one.

record Risk(string Name, int Probability, int Impact, string Response)
{
    public int Score => Probability * Impact;


    public string Level => Score >= 12 ? "high" : Score >= 6 ? "medium" : "low";
}
// by score; on a tie, the bigger impact goes first
static List<Risk> Ranked(List<Risk> risks) =>
    risks.OrderByDescending(r => r.Score)
        .ThenByDescending(r => r.Impact)
        .ToList();

The risks from the review of the shop:

List<Risk> risks = [
    new Risk("cycle orders ⇄ payments", 4, 3, "reduce: an event instead of a call back"),
    new Risk("payments have no timeout and slow orders down", 3, 5, "reduce: a timeout and a breaker"),
    new Risk("the domain calls web", 4, 2, "accept: fixed with the next pricing change"),
    new Risk("restoring from backup was never tried", 2, 5, "reduce: a test restore every month"),
    new Risk("only one person knows the 2009 warehouse", 3, 4, "reduce: a second person and a write-up"),
    new Risk("the PDF library for invoices is unmaintained", 2, 2, "accept"),
];
foreach (var r in Ranked(risks))
    Console.WriteLine($"{r.Score,2} {r.Level,-6} {r.Name} → {r.Response}");
Output
15 high   payments have no timeout and slow orders down → reduce: a timeout and a breaker
12 high   only one person knows the 2009 warehouse → reduce: a second person and a write-up
12 high   cycle orders ⇄ payments → reduce: an event instead of a call back
10 medium restoring from backup was never tried → reduce: a test restore every month
 8 medium the domain calls web → accept: fixed with the next pricing change
 4 low    the PDF library for invoices is unmaintained → accept

There are four responses:

  • avoid: remove the cause, for example do not ship the feature;
  • reduce: the probability or the impact, for example a timeout and a circuit breaker from "When the neighbour is slow";
  • transfer: to someone else, for example insurance or a contract with the provider;
  • accept: consciously and in writing, with a date for another look.

The numbers from 1 to 5 are a judgement, not a measurement. 12 is not "twice as bad" as 6. They are there to sort and to start the conversation, not to calculate with.

OrderByDescending and ThenByDescending are stable: risks with equal score and impact stay in the order they were written.

Try it

Predict the output

Four risks from another review, sorted with the same ranked. What does the program print?

List<Risk> risks = [
    new Risk("the email provider changes its API", 3, 2, ""),
    new Risk("the site certificate is renewed by hand", 3, 3, ""),
    new Risk("the warehouse goes down on Black Friday", 2, 5, ""),
    new Risk("monthly reports are late", 5, 1, ""),
];
foreach (var r in Ranked(risks))
    Console.WriteLine(r.Score + " " + r.Name);
Answer

The idea

When the review does harm

A review is a tool, not a ritual. It does harm when:

  • it is an exam for the team. A review that does not invite the people who wrote the code finds culprits, not risks. They know why things are the way they are.
  • the fitness function fails from noise. A flaky check (a load test on a shared machine) fails for no reason and gets switched off. One stable rule guards more than ten flaky ones.
  • the baseline only grows. Then it is a list of excuses. Watch its size over time.
  • the register sits in a folder. A risk with no owner and no date for another look is a forgotten risk.
  • the imports are the whole truth. They are not. A shared database table, a call over HTTP and a queue connect modules without an import. Modules that read each other's tables ("Monolith, modular monolith, services") do not show in the import graph.
More precisely, if you are curious

The tools. Nobody writes the dependency check from scratch. Java has ArchUnit (the rules are JUnit tests) and jdeps in the JDK; C# has ArchUnitNET and NetArchTest; Python has import-linter; JavaScript has dependency-cruiser and madge (for cycles). All of them pull the graph from the code and check rules like the ones in this lesson, including a baseline: in ArchUnit it is called FreezingArchRule.

How many cycles are there? The search in this lesson finds one arrow back for each cycle it meets. That is not the number of all cycles: if you run the review after domain/pricing -> service/payments, the path to payments goes through pricing, and the small cycle "orders ⇄ payments" is not counted separately. Tools look for strongly connected components (Tarjan's algorithm): the groups of modules where every one reaches every other. The question "which modules are in a cycle" is more useful than "how many cycles".

Review methods. ATAM (Architecture Tradeoff Analysis Method, from the SEI) is a formal review with quality scenarios and the stakeholders. The C4 model is a way to draw the architecture at four levels of detail, so that the diagram answers the reader's question.

Exercise

Cycles in the check too

Solve this one in your editor.

Add a second rule to the fitness function: the number of cycles must not grow above the cycle baseline (2 today). Write CheckAll(deps, baseline, cycleBaseline), which prints all findings and at the end whether it fails. Run it with a commit that adds data/orders -> service/orders: the data layer calls the service to send an email after a save.

Output
✗ new upward dependency: data/orders -> service/orders
✗ cycles: 3, baseline: 2
FAILS

More exercises

  • Who is in a cycle. Instead of the number of cycles, keep the set of modules that are in some cycle. Which rule is harder to get around?
  • Modules in a layer. Add a rule: service/payments must not call service/orders directly, only through an event (the lesson "Events and queues"). How do you put it in the baseline until the cycle is fixed?
  • The register. Give every risk an owner and a date for another look. Print the risks with a score of 12 or more that have no owner.

Test

Check yourself

1. Where does the review take the real dependencies between modules from?
2. The search reaches a module that is grey. What does that mean?
3. Why does a fitness function for an inherited system use a baseline?
4. Two risks score 12: probability 4 × impact 3 and probability 3 × impact 4. Which one comes first in the register from the lesson?
5. Which of these is a fitness function?

Made by Ivo Gergov as part of Evo Tech's campaign for accessible programming education ·