Introduction

If you read my last post on CITYdata, a pattern-based middleware built upon the Observer, Producer-Consumer, and Publisher-Subscriber design patterns, this one zooms into a single component: producers. As customer demand grows, so does the code; and with it, maintainability problems caused by structural defects accumulate silently over time.
Not all defects throw exceptions. Duplicated logic, dead code, and tangled responsibilities can compile and run correctly while quietly raising the cost of every future change. Normal build and test processes do not necessarily catch them, and without documentation or the original developer around, future maintainers may have to rediscover the design intent or guess what is safe to touch.
This post walks through a running example: the refactoring of a base producer class and its subclasses, in which we addressed three structural defects.

Duplicated Logic: One Job, Two Names

Code duplication is a common structural defect where the same or similar behavior is implemented in multiple places, often with slight variations. It causes the violation of the DRY (Don't Repeat Yourself) principle, and typically happens when existing code is copied and adapted for a new case. Duplication increases maintenance effort, as changes must be applied consistently across all copies. Because duplicated code still compiles and runs normally, these problems can remain unnoticed.
The base producer is called MainBaseProducer . In this first defect, MainBaseProducer fetches bytes from either an HTTP endpoint or the local filesystem through two separate private methods.

Old implementation

private void doHTTPRequest(OutputStream outputStream) throws IOException { ... }
private void readFile(OutputStream outputStream) throws IOException { ... }

Both solely pushed the bytes into a caller-provider OutputStream . Conceptually, they did the same job (getting bytes from wherever filePath), under two names, with fetchFromPath() branching between them. 

protected OutputStream fetchFromPath() {
		...
		if (this.filePath != null && this.filePath.contains("://") && this.fileOptions != null) {
				doHTTPRequest(outputStream);
		} else {
				readFile(outputStream);
		}

Refactored implementation

We refactored the code above by collapsing both paths behind a single seam:

private InputStream openInputStream() throws Exception {
		if (this.filePath != null && this.filePath.contains("://") && this.fileOptions != null) {
			return openHttpStream();
		}
		return openFileStream();
	}

We renamed fetchData to locateDataDirectory() , we made it private as its only purpose is to support path resolution, and actually wired it into the read path via resolveFilePath 

private Path resolveFilePath() {
		Path direct = Paths.get(this.filePath);
		if (Files.exists(direct)) {
			return direct.toAbsolutePath().normalize();
		}
         ...
		
		for (String propertyKey : java.util.List.of("routePath", "test.routePath")) {
			...
		}
		return direct;
	}

Now, every fetch passes through this method. There is now a single place where the data source is determined. So a fix applied there applies everywhere automatically. Consistency is enforced by design rather than relying on someone's memory.

Dead-But-Computed Logic

Dead code is not always code the compiler can detect as unreachable. It can also be logic that executes and produces, but whose result is never used. This code adds unnecessary complexity and can easily mislead the maintainers into thinking it is part of the active logic. Because it is often compiled and passes tests, it can remain unnoticed and waste time during future maintenance. In our running example, MainBaseProducer had a fetchData() method that read the data route path property (identified with a propertyKey) and resolved a Data fallback directory.

Old implementation:

public Path fetchData() {
// reads a data route path, walks a list of candidate "Data" folder locations, and returns a resolved Path
... 
}

But the only method that reads files from disk ignored it entirely:

private void readFile(OutputStream outputstream) throws IOException {
Path path = Paths.get(this.filepath) // no call to fetchData()
Files.copy(path, outputStream)
}

Refactored implementation:

We solved this defect by changing the logic. We renamed fetchData() to locateDataDirectory() , we made it private as its only purpose is to support path resolution, and actually wired it into the read path via resolveFilePath 

private Path resolveFilePath() {
		Path direct = Paths.get(this.filePath);
		if (Files.exists(direct)) {
			return direct.toAbsolutePath().normalize();
		}
         ...
		
		for (String propertyKey : java.util.List.of("routePath", "test.routePath")) {
			...
		}
		return direct;
	}

Now, the computed value is the value actually used, closing the gap between what the code appears to do and what it does.

Tangled Responsibilities

When a subclass overrides an entire method just to change one small part, it is a sign that responsibilities are not well separated. Instead of reusing the shared behavior and changing only what differs, the subclass duplicates the whole method. As a result, maintenance becomes harder because changes to the shared behavior must be manually applied to each subclass. It also obscures what is specific to each subclass, making the code harder to understand.

Old implementation

One of the concrete producers in our codebase is implemented by the CSVBTUProducer class. It is a subclass of CSVBaseProducer class, which contains the common logic for producers that fetch CSV data from a file. However, CSVBTUProducer overrides the fetch() method from CSVBaseproducer class entirely to parse the fetched data. As a result, it reimplements the whole method instead of only handling its specific behavior, which is to skip the header row. 

@Override
public void fetch() {
		try {
			ByteArrayOutputStream outputStream = (ByteArrayOutputStream) this.fetchFromPath();
			String csvString = outputStream.toString(StandardCharsets.UTF_8);
			String[] lines = csvString.split("\\R");
			ArrayList<String> csvLines = new ArrayList<>();
			for (int i = 1; i < lines.length; i++) {
				String line = lines[i].trim();
				if (!line.isEmpty()) {
					csvLines.add(line);
				}
			}

			this.setResult(csvLines);
			this.applyOperation();

		} catch (Exception e) { ... }
	}

Refactored implementation

We have restructured CSVProducer class around the template method pattern. Therefore, fetch() now handles the common steps, and defers the parts that vary to overridable methods; in the case of our example, beforeFetch() and parseLines() . CSVBTUProducer only overrides the parts that are specific to it.

@Override
protected void beforeFetch() {
    ...
}
			
@Override
protected ArrayList<String> parseLines(BufferedReader reader) throws IOException {
		ArrayList<String> csvLines = new ArrayList<>();
		reader.readLine();
		String line;
		while ((line = reader.readLine()) != null) {
			String trimmed = line.trim();
			if (!trimmed.isEmpty()) {
				csvLines.add(trimmed);
			}
		}
		return csvLines;
}

This new implementation clearly separates responsibilities, making the code easier to understand and, moreover, easier to maintain.

Conlusion

None of the structural defects described above caused compilation errors, test failures, or production incidents on their own. That's precisely why they are worth taking seriously. We walked through our middleware codebase to show how we identified and addressed these issues. Addressing them wasn't about adding features, but about reducing the cost of future changes, whether for system evolution or quality assurance.
Stay tuned for more updates on our middleware, openly accessible here