Is this an overuse of Java asserts?

Question

I'm trying to get to grips with the use of the assert keyword in Java. As I understand it, the correct case is for verifying things that should always be true.

I'm worried that I'm overusing asserts, however.

Here's a sample:

private BodyParams() {
    assert revokedDoc != null : "revokedDoc must not be null";
    assert revokedDoc.getStatus() == DocumentStatus.Revoked : "document is not revoked";
    assert !isBlank(revokedDoc.getDocType()) : "docType should not be blank";
    assert revokedDoc.getIssuedDate() != null : "doc should have issue date";
    assert revokedDoc.getSendingOrg() != null 
            && !isBlank(revokedDoc.getSendingOrg().getName()) 
            : "sending ord should exists and name should ne populated";

    if (registeredUser) {
        assert revokedDoc.getOwner() != null 
                && !isBlank(revokedDoc.getOwner().getFirstName()) 
                : "owner should exists and first name should be populated";

        this.ownerFirstName = revokedDoc.getOwner().getFirstName();
        this.docUrl = Application.PUBLIC_HOSTNAME 
                + controllers.routes.DocumentActions.viewDocument(
                    revokedDoc.getId()
                ).url();

    } else {

        this.ownerFirstName = null;
        this.docUrl = null;

    }

    if (revokedDoc.getStatus() == DocumentStatus.Available) {
        assert !isBlank(revokedDoc.getFriendlyName()) 
                : "friendly name should not be blank for picked-up docs";

        this.friendlyName = revokedDoc.getFriendlyName();

    } else {
        this.friendlyName = null;
    }

    this.docType = revokedDoc.getDocType();

    this.issueDate = revokedDoc.getIssuedDate();

    this.issuerName = revokedDoc.getSendingOrg().getName();
}

In this example, it is assumed that the revokedDoc field came from the database, and correct validation was performed when it was inserted. These asserts test that assumption. Is this overkill?

edit: I should mention that this is only for development code. Assertions will not be enabled in production. I'm using the assertions to ensure that data that will be known good data from a trusted source in production behaves itself in development

Answer 1

Assert is really useful to perform that should always be true inside an library or a module. It is intented to verify invariants ( control flow, internal, etc.) in your code, and it is a bad idea to use it to enforce correct use of your code (you have exceptions for that).

As a consequence, your public interface should never be based on assert : when you have a public method and you want to check input parameter, it is generally better to throw an IllegalArgumentException.

Here are some good documentation about asserts.

In your example, I think you should use exceptions instead of asserts. It's not a bad idea to perform some validity checks on data coming from a database (even if it has been validated on input) but assertion might be disabled in production code and you have to think on how you should handle such malformed content.

Answer 2

It does not look right. To simplify there are two broad categories of problems that can arise and require checking the validity of a variable:

• Your method receives or uses an argument that could possibly not be what you expect and your method should have appropriate argument checking and throw an IllegalArgumentException or NullPointerException or whatever if required. Example: the client code has passed in a null argument and you have no control over that code
• Your method uses some of the class internals and you should have appropriate unit tests to make sure that those internals are always consistent and that your methods can use them without additional checks.

In your case, the method that creates the revokeDoc object should make sure it is in a valid state after creation and take appropriate action otherwise, for example throw an exception and roll back any changes. That way your BodyParams method can just use the object without all those asserts which clutter your code at the wrong time: if revokeDoc is not consistent it is probably too late to do something about it and should have been detected earlier.

Related post: Exception Vs Assertion

Answer 3

This could be an opinionated question. However, I'd go with the following things to decide:

1. Is this method exposed to outside world (via an interface, JAR file, user input field or anywhere where you could get inputs from a source that is not in your control) - then I should have a valid actual check which would result in an exception.
2. Am i relying on assertion for my correct execution of the code? If so, I shouldn't. At runtime, assertions are meant to be disabled.
3. Is this assertion always true? and if yes, am I going to use it on the off case for just debugging - then yes, use an assertion in place of a code comment. When something goes bad, enable the assertions and figure out what's wrong.

Answer 4

You need to consider two scenarios: development code and production code.

Since Java's assert statement is disabled by default (and adds only little overhead by checking a global static flag which is enabled by passing -ea to the VM), I would not consider this overhead since it helps you detect issues early during your development phase (assumed that you have enabled assertions in your development environment).

On the other hand, you say "... Correct validation was performed when it was inserted ..." - so, how do you know that the value has not been changed in the database meanwhile? If security matters for your system (I am just assuming it does), one basic pattern is that you must not trust anything which you get from the outside. Means, validate values you read from the database - but, in that case, assert is not the proper tool. Use normal validation code and exceptions for that.

Answer 5

The best practice, acording to OO metology is to check the params you receive. And create regulars checks for others. Should in your case you should get something like this:

private BodyParams(revokedDoc)
[...]
asserts of the params

if(isBlank(revokedDoc.....)

All the assets looks good, and is the way to make sure the method has everything to work. But they should be to make an aide of what's going on wrong, not to make your program work.