slopfront
Reddit, read-only. No JavaScript. No cookies. No tracking.
preferences

Optional and Test Driven Design damage?

7 points · submitted Feb 2018 by u/_dban_ · archived

Here's a question for /r/java. How many of you guys are using SONAR to guide your testing agenda? We actually have set thresholds in our CI environment to fail builds if line and branch coverage fall below certain thresholds. Have such efforts effected (or damaged) how you write code?

For example, we are getting ResultSet objects mapped into Object[] arrays, which we then map into DTOs. Obviously, it would be better to skip the Object[] step and just go straight to DTOs. But for the sake of argument...

Consider the following code:

Option[] results = ...
ResultDTO dto = new ResultDTO();
dto.setSomeOptionalID(results[0] == null ? null : ((Number) results[0]).longValue());

SONAR will penalize you on branch coverage if you don't have a test for the null case and the non-null case.

I was considering replacing that with the following:

Option[] results = ...
ResultDTO dto = new ResultDTO();
Optional.ofNullable((Number) results[0]).map(Number::longValue).ifPresent(dto::setSomeOptionalID);

In this case, I can have a single test case for the non-null case, and SONAR won't penalize me on branch coverage.

On one hand, this illustrates the benefits of functional programming. It adds safety by reducing the number of branches.

On the other hand, this could be considered misusing Optional, introducing unnecessary objects, and damaging the design of Java code in the pursuit of better branch coverage numbers.

What do you guys think?

19 comments · 71% upvoted · other discussions

comments

[deleted] 10 points · Feb 2018

Technically, you're not reducing the number of branches, you're just hiding one branch inside Optional.

A better solution might be to add another constructor for your ResultDto:

public ResultDTO(Option[] results) {
  this.someOptionalId = results[0] == null ? null : ((Number) results[0]).longValue();
}

You can test both branches in this constructor easily.

Or make a another public mapping function that takes the Option[] and returns a ResultDTO. Again, you can test that separate function easily.

orion78fr 1 points · Feb 2018

What if results is null ?

_dban_ [OP] -2 points · Feb 2018

you're just hiding one branch inside Optional.

That reduces the number of branches, because due to the restricted semantics of the Optional type, the program now has less degrees of freedom. map and ifPresent work in very specific ways, while conditional constructs like ?: can work in any possible way.

You can test both branches in this constructor easily.

The question I'm posing is regarding how satisfying SONAR guides choice of code structure, and if such choices introduce "design damage" to satisfy tests or are valid in their own right. DHH posted a famous rant about this (is hexagonal design good in its own right, or is imposing hexagonal design on Rails for testability "design damage").

[deleted] 4 points · Feb 2018

Well, if you look at it from an input/output perspective, there's 2 input types (null and not-null) which map to two output types. In the abstract, full coverage requires that you test both code paths.

However, the tool in question doesn't detect that Optional.of(xxx).getOrElse(zzz) is the same as the ternary operator and a null check.

It's entirely possible that a future update might be able to detect and flag this as requiring two tests.

_dban_ [OP] -1 points · Feb 2018

Well, if you look at it from an input/output perspective

The point is that switching to Optional lets me write less test cases to pass SONAR, as in, I'm letting metrics drive how I write my code and how many tests cases I have to write.

More philosophically, extracting functions, using DTO classes instead of Map or using constructs like Optional instead of conditionals, let you get away with writing less tests because you're operating at a higher level of abstraction. It's like the static typing vs. dynamic typing argument - you need more tests for a dynamically typed program because the type system does not provide enough information to the compiler to eliminate a large number of tests.

It's entirely possible that a future update might be able to detect and flag this as requiring two tests.

That is not how SONAR's branch coverage works, which is based on cyclomatic complexity. The Optional version doesn't have any boolean conditions that can generate multiple branches in the local flow, compared to the boolean condition results[0] == null.

[deleted] 2 points · Feb 2018

I don't really agree with that. I think the cyclomatic complexity is the same in both cases. There are two code paths.

In basic Java, you have objects and control structures, and they're generally separate things. Code coverage tools work by examining the control structures to determine flow, while ignoring objects. But monadic objects like Optional are objects and control structures in one. As such, they aren't detected as control structures by the code coverage tool.

To me, this situation would be like SONAR ignoring classes which start with the letter 'A', and then you asking "Should I name all my classes starting with A in order to get a better score?' The answer is no. The tool is limited, and 'gaming' or exploiting the limits of the tool is not a good idea.

_dban_ [OP] -1 points · Feb 2018

I think the cyclomatic complexity is the same in both cases.

It isn't. You can do the math.

There are two code paths.

Not by cyclomatic complexity analysis.

Code coverage tools work by examining the control structures to determine flow, while ignoring objects.

Not exactly. Objects are irrelevant for control flow. Code coverage is looking at control structures and functions (which are always associated with an object in Java, but that is not relevant to cyclomatic complexity analysis).

Functions decrease complexity by decreasing degrees of freedom. Every if statement adds edges to the local code path.

However, when you pass over a function, that function's own complexity is not considered in the complexity analysis. Functions don't add edges to the control flow graph.

That is because once you are in the function, you can no longer interact with the code path that lead to it (excepting of course global context, but that is not a part of cyclomatic complexity analysis). This decreases overall complexity of the system by decreasing degrees of freedom.

Thus, using Optional, you are making a series of function calls. You aren't adding any edges to the control flow graph, and thus the code is less complex. The downside is more limited expression because you have to play by Optional's rules.

With a conditional, you are adding edges to the control flow graph. But you get more freedom of expression, at the price of increased complexity.

WikiTextBot 1 points · Feb 2018 · btproof

Cyclomatic complexity

Cyclomatic complexity is a software metric, used to indicate the complexity of a program. It is a quantitative measure of the number of linearly independent paths through a program's source code. It was developed by Thomas J. McCabe, Sr. in 1976.

[ PM | Exclude me | Exclude from subreddit | FAQ / Information | Source | Donate ] Downvote to remove | v0.28

WikiTextBot 1 points · Feb 2018 · btproof

Cyclomatic complexity

Cyclomatic complexity is a software metric, used to indicate the complexity of a program. It is a quantitative measure of the number of linearly independent paths through a program's source code. It was developed by Thomas J. McCabe, Sr. in 1976.

[ PM | Exclude me | Exclude from subreddit | FAQ / Information | Source | Donate ] Downvote to remove | v0.28

jonhanson 3 points · Feb 2018

chronophobia ephemeral lysergic metempsychosis peremptory quantifiable retributive zenith

_dban_ [OP] 1 points · Feb 2018

Like I said, the point was to make SONAR happy without having to write more tests, and the resulting use of Optional which could be considered "damage", depending on your perspective.

In this case, as I also mentioned, another API is returning the result set as List<Object[]>, which would require redesign to fix. I would rather use JdbcTemplate to directly map the ResultSet to a DTO.

jonhanson 3 points · Feb 2018

chronophobia ephemeral lysergic metempsychosis peremptory quantifiable retributive zenith

WikiTextBot 1 points · Feb 2018 · btproof

Goodhart's law

Goodhart's law is an adage named after economist Charles Goodhart, which has been phrased by Marilyn Strathern as: "When a measure becomes a target, it ceases to be a good measure." This follows from individuals trying to anticipate the effect of a policy and then taking actions which alter its outcome.

[ PM | Exclude me | Exclude from subreddit | FAQ / Information | Source | Donate ] Downvote to remove | v0.28

GeorgeMaheiress 3 points · Feb 2018

All automated code metrics are naive and should not be carelessly optimized for. If you are writing worse code in order to appease your code coverage requirement, stop and think about what value you want to get from test coverage. If there's a valuable test you can add, do that instead. If there's no valuable test to add, consider relaxing your code coverage requirement.

cogman10 3 points · Feb 2018

I don't understand why you wouldn't just write the test.

ickysticky 2 points · Feb 2018

I expect that the reason the branch coverage changes is because you have actually changed what the code does.

In your first code snippet dto.setSomeOptionalID is always called, sometimes with null, sometimes with a value.

In the second code snippet you only ever call dto.setSomeOptionalID when the value is not null. This would be equivalent to this code

Option[] results = ...
ResultDTO dto = new ResultDTO();
if (results[0] != null) {
    dto.setSomeOptionalID(((Number) results[0]).longValue());
}

Which as you can see doesn't have an else branch. If you want your optional code to be equivalent to the original it should be like this

Option[] results = ...
ResultDTO dto = new ResultDTO();
dto.setSomeOptionalID(Optional.ofNullable((Number) results[0]).map(Number::longValue).orElse(null));

Which seems like a more verbose version of the ternary operator.

_dban_ [OP] 1 points · Feb 2018

In your first code snippet dto.setSomeOptionalID is always called, sometimes with null, sometimes with a value.

The intent of the code is the same as your first code block, but I wanted to implement the intent as a one-liner, which requires specifying null when you use the ternary. Since the default value is null, the effect is the same.

In order to get full branch coverage, you would still need two tests for the first code block. Based on the number of conditions in the if, SONAR would count the number of possible branches that need to be covered. In this case, since results[0] != null can have two possible values, SONAR would penalize you on branch coverage if your tests didn't hit both cases.

nutrecht 1 points · Feb 2018

I don't really understand how this is 'damage'? It's your choice to handle this yourself instead of using some kind of standard library or shared library. If you write code yourself instead of (re)using something it's more work, partially because you also need to test that work.

Whether you use a monadic approach or 'just test' it, it doesn't really matter to me. That's personal preference. Both methods could be generalised into utility functions you only need to test once. And sonar is right to complain if you don't.

_dban_ [OP] 1 points · Feb 2018

Damage in the sense that DHH meant it.