Implement analyzer for cars-assemble concept exercise

[chiarazarrella]

Closes #103

In the design.md of the exercise there is such rule:

• informative: If the solution has if/else-if statements in the productionRatePerHour method, inform the student that creating a helper method to calculate the succes rate might make their code easier to understand.

However, I reused an existing comment: method_too_long, which I think it fits with the request. The only difference is that such comment is of type ACTIONABLE, meanwhile in the design.md file it is requested to be INFORMATIVE.

ToDo:

• Add website-copy comment
• Check comments #2380

[IsaacG]

+cc the @exercism/java maintainers

[kahgoh]

This method seems to check if the number 221 appears anywhere, but the comment in the design.md seems to suggest that it should check if the number appears more than once in the code:

If the solution is repeatedly hard-coding the value 221, inform the student that they could store this value in a field to make the code easier to maintain.

An alternate solution may to set it to a final variable (within a method), especially if it isn't used anywhere else. If the value appears only once, I think that could be fine.

[chiarazarrella]

What if the value appears only once, but there is no final variable declared within the method?
I think this is still a "magic number" and therefore incorrect. Because we do not understand what is the meaning of 221 solely.

Something like:

public double productionRatePerHour(int speed) {
        return 221 * speed * successRate(speed);
    }

[jagdish-15]

The current version correctly flags magic numbers. Since this method (useMagicNumber(MethodDeclaration n)) analyzes only a single MethodDeclaration, which represents one method and not the entire CompilationUnit, any occurrence of the number within that method is rightly flagged as a magic number. Therefore, no change is needed!

[glennj]

Are you saying that this is still magic?

public double productionRatePerHour(int speed) {
    int cars_per_hour = 221;
    return cars_per_hour * speed * successRate(speed);
}

[jagdish-15]

Makes sense! Apologies, I completely missed this angle. Indeed, this is not a magic number situation!

[jagdish-15]

What do you think of something like this?

private boolean useMagicNumber(CompilationUnit node) {
    List<VariableDeclarator> declarations = node.findAll(VariableDeclarator.class);

    long declarationCount = declarations.stream()
            .filter(vd -> vd.getInitializer().isPresent()
                    && vd.getInitializer().get().toString().equals(MAGIC_NUMBER))
            .count();

    long literalUsages = node.findAll(IntegerLiteralExpr.class).stream()
            .filter(lit -> lit.asString().equals(MAGIC_NUMBER))
            .count();

    return declarationCount == 1 && literalUsages == 1;
}

[glennj]

I'm not familiar with how the java analyzer works. I read this as saying, for some node in the AST, if the magic number appears it must be part of a variable declaration. Is that right?

[jagdish-15]

Yes, and it should be declared only once, with the number not used anywhere else except in that declaration! Right, @kahgoh?

Thanks for catching the above, though!

[kahgoh]

Yes, and it should be declared only once, with the number not used anywhere else except in that declaration! Right, @kahgoh?

Uh ... if I go back to what was written in the design.md, it would be no, I don't think that's quite right. The design.md doesn't mention anything about magic number at all - just that the number 221 shouldn't appear in the code more than once in the code.

To be honest, I thought this solution looked ok (assuming this is the only place where 221 appears):

public double productionRatePerHour(int speed) {
        return 221 * speed * successRate(speed);
}

If 221 should only be in the declaration, what about the other numbers? The reference solution shows there are also other numbers in the solution like 10, 9, 5, 0.77, etc.

[kahgoh]

Something here seems strange here. You get a list of IfStmts, start iterating, but return true / false depending on whether the first if statement in the list is has an else block? What about the other if statements?

Even if you really did mean to check just the first if statement, I think the not-using-helper-method test would fail if productionRatePerHour used multiple returns like this community solution:

public class CarsAssemble {

    public double productionRatePerHour(double speed) {
        double baseProductionRate = 221;

        if (speed >= 1 && speed <= 4) {
            return speed * baseProductionRate;
        } else if (speed >= 5 && speed <= 8) {
            return speed * baseProductionRate * 0.9;
        } else if (speed == 9) {
            return speed * baseProductionRate * 0.8;
        }
        return speed * baseProductionRate * 0.77;
    }

    public int workingItemsPerMinute(int speed) {
       return (int) productionRatePerHour(speed)/60;

       
    }
}

[kahgoh]

I'm concerned that reusing the MethodTooLong could be confusing for the students. There's nothing in the comment that hints students should move the if statement. I think its really hard for the students to tell that they should move it a helper method because it only tells them to make the method smaller, but we actually them to specifically move the if statement into a separate method. The actionable level may be too high for this comment too.

[IsaacG]

@chiarazarrella Are you still working on this?

[chiarazarrella]

Sorry, been busy with my master thesis. I will work on it for the next weeks! :)