Forem: Roland Weisleder

Refactoring Legacy Code: Can We Trust Existing Tests?

Roland Weisleder — Fri, 01 Mar 2024 08:43:22 +0000

Recently, I saw an interesting question on LinkedIn: "When making a moderately large refactor, should we trust the tests we already have, or should we drive our refactor with new tests?"

As an independent consultant who is bringing legacy systems into the future, of course this question is of great interest to me. First of all, in legacy code, tests do not always exist or the code coverage is not that high. My first step is always to verify the existence of tests and evaluate their coverage. It's important to ensure that all critical areas, especially those that will be refactored, are well covered by tests. If not, then adding or improving tests is essential.

Understanding the tests is as important as having them. If they don't accurately reflect the business requirements, I will then focus on refining them or creating new tests. This approach not only helps to understand the codebase, but also to build confidence in its reliability. In essence, getting to know the code through testing is important to me.

When it comes to the refactoring itself, the goal is to change the structure without changing the business logic. This is where the IDE and automated refactoring tools are invaluable. They're less likely to make mistakes than moving code around by hand.

So, to answer the original question: First, we need to have enough tests we can trust. Trust can be gained by having sufficient knowledge about the code unit to be refactored. This can be done, for example, by writing new tests. Automated tools help us to make the refactoring itself less error-prone.

This post was originally published on LinkedIn. Follow me there for more tips on working with legacy systems.

Have You Seen Your Tests Fail?

Roland Weisleder — Thu, 05 Oct 2023 06:01:49 +0000

You might think this is a rhetorical question, since Test-Driven Development (TDD) is promoted everywhere. According to TDD, we first write a failing test for a new requirement. Then, in a second step, we write just enough code to make the test pass. Finally, we clean up the code and have the tests as a guarantee that nothing breaks. And then the cycle starts all over again.

But this test-first approach is not used by everyone. And it's even less likely to be used consistently all the time.
A second point is that "writing just enough code" is sometimes harder than expected. At least for me, it often happens that I already have a rough idea of what the code will look like in the end. Without realizing it, I write more code than I actually need for the current test.

But how can we verify that our tests fail when a functionality breaks? How to test our tests?

One approach is Mutation Testing. In this approach, bugs are deliberately introduced into the production code. For example, conditions are inverted or entire statements are removed. The goal is to find out if the tests fail and thus find the built-in bug.

For Mutation Testing, there are libraries that automate the process. But the execution can be very time-consuming. Sometimes manual Mutation Testing is sufficient by modifying the newly written code and seeing if the newly written tests fail.
To test my tests, I usually do exactly this: Just change the code and deliberately introduce errors. If the tests then fail with a helpful error message, even better. That gives me more confidence in my tests.

If the created mutations, automated or manual, are not detected, there may be room to improve the tests, or we may have more code than necessary. However, we should also make sure that production code and test code are not too tightly coupled. Then we run the risk of tests unnecessarily getting in the way of major changes or refactorings.

Therefore: Try to break your code and Happy Testing!

The Flag Parameter Anti-Pattern

Roland Weisleder — Thu, 03 Nov 2022 12:18:04 +0000

While implementing a new feature, I came across the flag parameter anti-pattern in two unrelated places. I would like to take this as an opportunity to take a closer look at this anti-pattern.

A simple example

Lets assume we want to load all PDF documents via an API method. This could look like this:

public interface DocumentService {

    Document[] loadDocuments();
}

It Starts Smelling

In the next step we need a method to load all TIFF documents. Since the difference in implementation might be only one line of code, the result might look like this:

public interface DocumentService {

    /**
     * @param loadTiff true: loads TIFF documents - false: loads PDF documents
     */
    Document[] loadDocuments(boolean loadTiff);
}

The implementation is also modified very quickly with an additional if block:

public Document[] loadDocuments(boolean loadTiff) {
    int format = 1; // load PDF documents by default
    if (loadTiff) {
        format = 2; // except when TIFFs are requested
    }
    ...
}

However, if we look at the client side of this API, we already notice an unpleasant smell:

Document[] documents = documentService.loadDocuments(false);

At this code point, can you tell what the meaning of false is without looking at the definition of the method? And what happens if we change this to true?

The Smell Begins to Stink

In the next step we also need a method to load RTF documents. Again, the difference in implementation is only one line of code. But how can we reuse the existing method without having to re-implement the logic? The flag parameter stands in our way! The boolean parameter can only take two states, true or false. (Okay, the clever Java developer changes this to a Boolean parameter and has true, false and null as choices).

However, the flag parameter also shows us something else: the single responsibility principle is violated here. In fact, this method currently has more than one responsibility. On the one hand, it knows how to load PDF documents. On the other hand, it also knows how to load TIFF documents. Or more generally: such a method does two different things - once on true and once on false.

Possible Solutions

Spontaneously I can think of two simple ways to get rid of the flag parameter.

Option 1: We define an interface for the document types to query for information. Then we can generalize the method by using the document type as a parameter. However, there is a possibility of unnecessary over-generalization of the business logic.

public interface DocumentService {

    Document[] loadDocuments(DocumentType documentType);
}

Option 2: We create a specific method for each possible domain context.

public interface DocumentService {

    Document[] loadPdfDocuments();

    Document[] loadTiffDocuments();

    Document[] loadRtfDocuments();
}

In both cases, from the client's side, this results in readable code:

Document[] documents = documentService.loadDocuments(DocumentType.PDF);
Document[] documents = documentService.loadPdfDocuments();

What About Setter Methods?

If we were strict, then we would also have to change

public class Action {

    private boolean enabled;

    public void setEnabled(boolean enabled) {
        this.enabled = enabled;
    }
}

public class Action {

    private boolean enabled;

    public void enable() {
        this.enabled = true;
    }

    public void disable() {
        this.enabled = false;
    }
}

However, we don't have to be more papal than the pope here. If I had to choose between

action.setEnabled(actionCheckBox.isChecked());

and

if (actionCheckBox.isChecked()) {
    action.enable();
} else {
    action.disable();
}

for the client code, I would stay with the first one.

Why Unit Tests Will Save You a Lot of Time

Roland Weisleder — Thu, 06 Oct 2022 11:20:43 +0000

When you as a developer hear that you should write unit tests, it sounds like more effort at first. The code you have written works, as you have already seen by trying it.

However, the world around it is constantly changing. And how can you be sure that the code still works despite changes from the outside?

Date Format?

Let's assume that in our program there is a function to parse strings as dates.

/**
 * @param input a date with format "dd.MM.yy HH:mm"
 */
public static Date parseDate(String input) throws ParseException {
    return DateFormat.getInstance().parse(input);
}

We use a class from the JDK here and even describe in the Javadoc which format the input parameter should have. Once we have tried this, what else can go wrong? Surely this will work forever.

Even after updating from Java 8 to Java 11, everyone would assume that the function still works as described, right? After all, we are only using a class from the JDK.

Let's do a test:

@Test
void parseDate_should_parse_String_to_Date() throws ParseException {
    Date parsed = parseDate("14.02.09 00:31");
    assertThat(parsed).hasYear(2009).hasMonth(2).hasDayOfMonth(14).hasHourOfDay(0).hasMinute(31);
}

With Java 8, the test is successful. Everyone would probably expect the same with Java 11. However, the result is:

java.text.ParseException: Unparseable date: "14.02.09 00:31"

    at java.base/java.text.DateFormat.parse(DateFormat.java:395)
    at org.example.Dummy.parseDate(Dummy.java:31)
    at org.example.DummyTest.parseDate_should_parse_String_to_Date(DummyTest.java:36)

With Java 11 the default pattern of DateFormat was changed from "dd.MM.yy HH:mm" to "dd.MM.yy, HH:mm". Now as a developer you have to make a choice: Adapt the implementation of parseDate so that the interface (see Javadoc) is fulfilled again? Or adapt the interface (Javadoc) and all callers to the new behavior?

Currency Format?!?

A second example inspired by the update from Java 8 to Java 11. Let's assume that in our program there is a function to format numbers as currency. For simplicity, let's again take a class from the JDK.

/**
 * Formats 13.37 as "13,37 €"
 */
public static String formatCurrency(double input) {
    return NumberFormat.getCurrencyInstance(Locale.GERMANY).format(input);
}

Then let's do a test for it:

@Test
public void formatCurrency_should_format_double_to_String() {
    String formatted = formatCurrency(13.37);
    assertThat(formatted).isEqualTo("13,37 €");
}

With Java 8 the test is successful. However, with Java 11 you get the following error:

Expecting:
 <"13,37 €">
to be equal to:
 <"13,37 €">
but was not.

Okay, solution: With Java 8 a normal space (ASCII code 0x20) was put between number and currency symbol. With Java 11, a non-breaking space (ASCII code 0xA0) is used. They both look the same, but they are not the same.

This change in the JDK then has the effect that formatCurrency(13.37).replace(" €", "") still returns the string "13.37 €".

Conclusion

These specific cases were deeply hidden in the system. After a Java update, no one would have noticed these changes, neither in a local test, nor in a customers' test system. This would be a typical candidate for "bugs that first become visible in the production system". Fortunately, in both cases there were unit tests for the affected parts.

Unit tests are always a good way to ensure that a function works as expected by the developer. Without unit tests you will only notice very late or too late when something does not work as expected.

By the way, if you try out the code you wrote, you could convert this "trying out" directly into tests, couldn't you? Then the testing is also much faster, even if other developers want to try something.