Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] Code style rule to favor while over do-while loops #2154

Open
linusjf opened this issue Dec 9, 2019 · 4 comments
Open

[java] Code style rule to favor while over do-while loops #2154

linusjf opened this issue Dec 9, 2019 · 4 comments
Labels
a:new-rule Proposal to add a new built-in rule

Comments

@linusjf
Copy link

linusjf commented Dec 9, 2019

Affects PMD Version:
6.20.0

Rule:
Related to WhileLoopWithLiteralBoolean

Description:
Suggest converting a do while loop to a while loop making the condition visible at the outset. This will be an optional feature permitting developers to exercise their preference for while loops as against do while.
The rule can be renamed, if needed.

Code Sample via https://stackoverflow.com/questions/25858163/how-to-convert-a-do-while-loop-to-a-while-loop

do
{
    System.out.println("Do you want to convert to Peso or Yen?");
    pesoOrYen = readKeyboard.nextLine();

    if(pesoOrYen.equalsIgnoreCase("Peso")||pesoOrYen.equalsIgnoreCase("Yen"))
    {
        notPesoOrYen = false;
    }
    else if (numAttempts < 2)
    {
        System.out.println("Sorry, but '"+pesoOrYen+"' is not a valid currency type. Try again:");
        notPesoOrYen = true; 
    }

    numAttempts++;

} while(notPesoOrYen==true && numAttempts < 3);
int numAttempts = 0;
boolean notPesoOrYen=true;
while (notPesoOrYen && numAttempts < 3) {
    System.out.println("Do you want to convert to Peso or Yen?");
    pesoOrYen = readKeyboard.nextLine();

    if (pesoOrYen.equalsIgnoreCase("Peso") || pesoOrYen.equalsIgnoreCase("Yen")) {
        notPesoOrYen = false;
    } else if (numAttempts < 2) {
        System.out.println("Sorry, but '" + pesoOrYen + "' is not a valid currency type. Try again:");
        notPesoOrYen = true; 
    }
    ++numAttempts;
};

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other]

@linusjf linusjf changed the title [java] WhileLoopWithLiteralBoolean can be modified to support only while loops. [java] WhileLoopWithLiteralBoolean can be modified to support while loops only. Dec 9, 2019
@adangel adangel added the a:new-rule Proposal to add a new built-in rule label Dec 9, 2019
@adangel
Copy link
Member

adangel commented Dec 9, 2019

I think, this should be a rule on its own, since WhileLoopWithLiteralBoolean is very limited. It only looks a loops where the condition is a boolean literal constant.

Let's create an example: This new rule should detect following snippet:

boolean someCondition = ...;
boolean someOtherCondition = ...;

do {
    // more code
} while (someCondition && someOtherCondition);

Hm... I'm not sure, what we would suggest here? Do-while loop execute the body at least once regardless of the condition. So just moving the condition to the outset will change the program...
The example from above with a while loop would look like this:

boolean someCondition = ...;
boolean someOtherCondition = ...;

// more code - the duplicated loop body
// usually you would extract the loop body into a method a call the method here

while (someCondition && someOtherCondition) {
    // more code
};

Did I understand you correct? I find it much easier to work with examples than with textual descriptions of what the rule should do/not do...

@linusjf
Copy link
Author

linusjf commented Dec 9, 2019

What do you need an example for?

While code examples may be useful, in this case it's not. Resetting a condition happens in the loop. If you're setting a condition outside a do while loop, it only comes into force at the end of the first loop within which the condition may already have been reset.

A while loop simply has the condition set to true at the outset if you wish to execute it at least once---which is exactly the case with the while true as well. If you expect it to run only once every time, you don't need a loop. Period. That's pretty much evident from your explanation for the existing rule. The condition is irrelevant (in that case).

I have a couple of examples in my source:

linusjf/LearnJava@06665a1#r36352553

Do you know of any exceptional instances where only a do while loop will do and nothing else?

I think this rule should include the while true case as a limiting of this rule and the while true rule can be deprecated and phased out. You don't need too many similar rules; it can become confusing to users.

@oowekyala
Copy link
Member

What do you need an example for?
I find it much easier to work with examples than with textual descriptions of what the rule should do/not do...

...

Besides, examples are needed to write tests, so at some point someone has to write them.

Anyway if I understand correctly, you want a new rule eg "DoLoopCanBeWhileLoop". AFAIK, do loops can always be replaced with while loops by duplicating the body before the loop. So I'm not sure how to formulate a rule that doesn't just flag all do loops. Your providing code examples would help.

Then, I don't understand the relation to WhileLoopWithLiteralBoolean, which has nothing to do with do loops. Also the title of the issue does not match the description, or the comments you posted.

If you expect it to run only once every time, you don't need a loop. Period. That's pretty much evident from your explanation for the existing rule.

This seems more suited for a distinct rule, eg "LoopStatementDoesNotLoop".

@linusjf
Copy link
Author

linusjf commented Dec 14, 2019

This rule will replace all do while loops with a while loop. As I said , it's for programmers who prefer while loops with the condition upfront and shun do loops. It would be controversial and thus I preferred it as an optional choice in the existing rule. It's obvious that if the rule is to replace all do while loops with while , no code samples are required. Code sample provided above.

LoopStatementDoesNotLoop sounds like a do while (false) loop to me. You'd never have something like that happen with a while loop. Isn't that case addressed with the existing rule? Is this a serious attempt at a rule in the first place?

The next-best thing would be to have a separate rule to convert all do while loops to whiles. What is PMD's policy on controversial rules? This would be an optional rule. You wouldn't want this in any default configuration.

@linusjf linusjf changed the title [java] WhileLoopWithLiteralBoolean can be modified to support while loops only. [java] WhileLoopWithLiteralBoolean can be extended to support while loops only. Dec 14, 2019
@jsotuyod jsotuyod changed the title [java] WhileLoopWithLiteralBoolean can be extended to support while loops only. [java] Code style rule to favor while over do-while loops May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:new-rule Proposal to add a new built-in rule
Projects
None yet
Development

No branches or pull requests

3 participants