Leap Analyzer Issue

[jagdish-15]

This PR addresses the issue raised by @kahgoh for the leap exercise!

[kahgoh]

Changes look ok so far. However, we're missing tests to prove this will work as intended (or at least changes to existing ones). There are a couple of tests failing in the CI.

For adding tests have a look in:

• tests/leap for tests run in Docker. You might also need to see AnalyzerIntegrationTest to add additional test cases.
• src/test/resources/leap and src/test/resources/analyzer for tests run with JUnit and Gradle

[jagdish-15]

All tests pass on my end, could you have a look when you have a moment?

[jagdish-15]

@kahgoh, are there any other issues you'd like me to address in this one?

[kahgoh]

@jagdish-15 It's looking good. There are a few things to tidy up and just thinking about whether we still want the analyzer to comment on the if statement.

[kahgoh]

This looks great. However, it just occurred to me we might also get an if statement version of this solution. For example:

if (year % 100 == 0) {
  return year % 400 == 0;
} else {
  return year % 4 == 0;
}

I think the analyzer will currently raise a comment about using the if statement. Do you think we should still be telling students to avoid if statement in this case?

[jagdish-15]

On one hand, I understand that it’s just a different way to express the same logic we already added using a ternary operator. But at the same time, it feels like a bit of overkill—especially when the same logic can be written in a single line using a ternary or logical operators.

I do have one suggestion—I'm not sure if this is even possible, but just putting it out there: does Exercism support different types of analyzer comments? Like maybe a way to leave a non-critical or informational-only comment for cases like this, where the solution is perfectly valid but there might be a more concise alternative? I’m not sure what the official terminology is (just guessing here), but something like this could be a nice middle ground.

[IsaacG]

Should the optional solutions be in a dig deeper article vs the analyzer?

[kahgoh]

Ok, let's leave it as it is currently.

I'm not sure if this is even possible, but just putting it out there: does Exercism support different types of analyzer comments?

To answer this one - Yes, there are different types of comments (essential, actionable, informative or celeboratory). The if statement comment is currently "actionable".

Should the optional solutions be in a dig deeper article vs the analyzer?

There is already an approach for this.

[jagdish-15]

Ok, let's leave it as it is for now.

Sounds good!

But if we do want to address it before the "Dig Deeper" section, one idea is to add an informative comment for optimal if statements, and keep the usual actionable feedback for non-optimal ones. That way, we're still guiding the user in the right direction.

We can check if an if statement is optimal by ensuring it doesn't return any boolean literals, and the "not more than 3 conditions" rule is already being handled by another method—similar to how we validate ternary expressions.

Let me know what you think and how you'd like me to proceed with this!

[SleeplessByte]

@jagdish-15 @kahgoh my two cents: leave as is, follow-up PR if necessary, because you probably want a bunch of test cases for that.

I do like the "informative" road. We do it a few times in te JS track, giving informative feedback about alternative paths.

[kahgoh]

@jagdish-15 @kahgoh my two cents: leave as is, follow-up PR if necessary, because you probably want a bunch of test cases for that.

I agree with leaving this for now.

[jagdish-15]

@SleeplessByte, I agree, we can keep this PR focused on the ternary issue that was raised, and avoid mixing in the if statement changes for now. If needed, we can create a separate PR to address the if statement scenario. In that PR, we could differentiate between valid solutions using if statements (as @kahgoh demonstrated above) and invalid ones, rather than raising a comment for all types of if statements as it currently does.

@kahgoh, let me know your thoughts on this approach!

[kahgoh]

In the src/test/resources/scenarios/leap, there is an UsingTernary file. I'd suggest renaming the file to make it easier to distinguish it from the OptimalTernary file.

[kahgoh]

Should this be checking if both sides are boolean literals? If only one side is a boolean, I'd assume the other side is some boolean expression (e.g. (year % 400 == 0) ? true : (year % 100 == 0) ? false : (year % 4 == 0)).

[jagdish-15]

Yep, this is checking whether either the then or else expression is a boolean literal. If either one is, it triggers the action. In the example you shared, the else expression is a ternary while the then is a boolean literal — so the condition still passes because we're checking both sides. But the opposite case (where then is a ternary and else is a boolean literal) is also possible, which is exactly why I made sure to check both!

Let me know if I’m thinking in the right direction here.

[kahgoh]

Sorry, I was trying to figure out whether line 79 should be (notice the && instead of ||):

if (node.getThenExpr().isBooleanLiteralExpr() && node.getElseExpr().isBooleanLiteralExpr()) {

I noticed the test for this case uses the expression:

(year % 400 == 0) ? true : (year % 100 == 0) ? false : (year % 4 == 0) ? true : false

But the proposed comment is:

Try returning the conditions directly instead of returning boolean literals (true and false).

Are we trying to say don't use a ternary expression to just return true / false (as in the (year % 4 == 0) ? true : false part)? I don't think it would make sense if it caught (year % 100 == 0) ? false : (year % 4 == 0) because the else part is another expression.

Another possibility is that we tell the students that it can be solved with just one ternary expression (instead of saying don't use boolean literals). This could be done by checking if there is more than one ternary expression (in a similar way to how we already check if there are more than 3 checks).

[jagdish-15]

Makes sense. We can change this message:

# avoid redundant ternary

Try returning the conditions directly instead of returning boolean literals (`true` and `false`).

to something like:

# avoid redundant ternary

Consider using a single ternary operator for the most efficient solution.

But then the title would need to be changed too, right? If it does, let me know what I should rename it to!

I would suggest the title to be something like "avoid multiple ternary".

Additionally, we’d need to update the logic to check that the solution uses only one ternary. The current implementation should still work, but this approach of checking the number of ternary operators makes more sense for the message and improves the overall understandability of the system.

The name of the scenario would also be changed to UsingMultipleTernary instead of UsingRedundantTernary, which was discussed before.

I’ll check this out over the weekend!

[jagdish-15]

Also, let me know if I should make these changes in this PR itself or create a follow-up PR for the rest, as @SleeplessByte mentioned!

[jagdish-15]

Yes, that makes sense. Sometimes we do use ternary operators to return boolean literals directly — for example:
(year % 100 == 0) ? false : (year % 4 == 0)
Even though this specific case wouldn’t be valid here, it shows that returning boolean literals via ternary operators isn’t always wrong.

So instead of telling students to avoid using ternaries to return boolean literals in general, it’s better to guide them with something more exercise-specific, like saying “this can be solved using just one ternary.” That’s more accurate and helpful in context.

Let me know if I’m on the right track and if this is what you meant!

[jagdish-15]

In the src/test/resources/scenarios/leap directory, there's a file named UsingTernary. I’d suggest renaming it to make it easier to distinguish from OptimalTernary.

What do you think about renaming it to something like UsingRedundantTernary?

[kahgoh]

What do you think about renaming it to something like UsingRedundantTernary?

I think that's ok since it aligns with AvoidRedundantTernary.

[SleeplessByte]

@kahgoh ping me if you need a guardian review once this is done. If I do not respond quick enough / no other guardian picks it up, feel free to profusely ping on Discord, for example in #maintaining.