cheptsov / AdvancedExpressionFolding

https://plugins.jetbrains.com/plugin/9320?pr=idea
140 stars 30 forks source link

Confused folding when check if a variable is null while it has a exception type #127

Open rofinily opened 5 years ago

rofinily commented 5 years ago

The original code:

Throwable exception = null;
if (exception != null) {
    throw exception;
}

The processed code:

assert exception == null;

Isn't it confused when reading?

ciscorucinski commented 5 years ago

What do you see wrong with this? What would make it more readable?

rofinily commented 5 years ago

assert exception == null means I assert this exp is true, or an exception would be thrown;

// it means I want to throw this exception if it is not null(or I will get an unexpected NPE)
if (exception != null) {
    throw exception;
}

Can you understand the difference between these two statements?

I think this kind of code needn't be processed, just let it be.

ciscorucinski commented 5 years ago

Those two expressions are the same. Just because you can express it in English differently, doesn't mean it is different

assert exception == null

This means, throw an exception if the condition is false

if (exception != null) {
    throw exception;
}

This means, throw an exception if the condition is true

So these two statements are the exact same in the same sense that [ 4 - 2 ] = [ 4 + ( -2 ) ]. They are the same.

rofinily commented 5 years ago
// equals to assert exception == null
if (exception != null) {
    throw new NullPointerException();
}

// the only difference is throw the checked variable itself
if (exception != null) {
    throw exception;
}
ciscorucinski commented 5 years ago

OK, now you brought up a valid difference. The point of this plugin is to make the code more readable.

You can basically refactor the original code into the assert statement. The assert statement is more simple to read and understand. then the if statement version. Simple and concise is important. It tries to keep the folded statement's meaning as close to the original as possible.

I would argue that many people would agree with the current implementation as being more than enough; however, if you absolutely need that difference to be shown at all time, then you can disable the "assert" fold in the settings.

Is that difference really that important that you need to have it shown at all times? I am sure that other aspects of your code is far more important since a simple mouse-hover can reveal the original code