JanusGraph / janusgraph

JanusGraph: an open-source, distributed graph database
https://janusgraph.org
Other
5.3k stars 1.17k forks source link

Replace unnecessary Preconditions by assert #2109

Open porunov opened 4 years ago

porunov commented 4 years ago

In many places Preconditions are redundant and have no any sense. Many Preconditions are running in production and are checking something that has no any sense. I am suggestion to replace unnecessary Preconditions by assert because assert is disabled by default and is commonly used only for testing purposes or hints in code. By using assert users will be able to enable those checks if they think they are needed, otherwise unnecessary checks will be disabled by default.

For example, place with the following check has no sense as we are throwing NPE when executing additions.isEmpty(), so checkNotNull is redundant.

Preconditions.checkNotNull(additions);
if (additions.isEmpty()){
    additions=null;
}

Thus, we can simply replace it with the following code:

assert additions!=null;
if (additions.isEmpty()){
    additions=null;
}

An example of needed checks might something like:

this.additions = Preconditions.checkNotNull(additions);
li-boxuan commented 4 years ago

Agree with the point that we can simply use assert to substitute Preconditions.checkNotNull in the following case,

Preconditions.checkNotNull(additions);
if (additions.isEmpty()){
    additions=null;
}

but slightly disagree with

place with the following check has no sense

It actually makes sense because it clarifies that to not accept any null value is by intention. Otherwise people might think null case is missed by mistake.

porunov commented 4 years ago

It actually makes sense because it clarifies that to not accept any null value is by intention. Otherwise people might think null case is missed by mistake.

Yes, I think I've put that statement wrong. I mean here that the check compiled instruction here isn't necessary. As from code perspective I completely understand why it was used (as a hint for developers / users). But that hint can be achieved without enabling a check in production (like assert, @NotNull annotation or javadoc).