-
-
Notifications
You must be signed in to change notification settings - Fork 25
Implement analyzer for cars-assemble concept exercise
#277
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package analyzer.exercises.carsassemble; | ||
|
|
||
| import analyzer.Comment; | ||
|
|
||
| /** | ||
| * @see <a href="https://github.com/exercism/website-copy/blob/main/analyzer-comments/java/cars-assemble/avoid_using_return_in_else_statement.md">Markdown Template</a> | ||
| */ | ||
| public class AvoidUsingReturnInElseStatement extends Comment { | ||
| @Override | ||
| public String getKey() { | ||
| return "java.cars-assemble.avoid_using_return_in_else_statement"; | ||
| } | ||
|
|
||
| @Override | ||
| public Type getType() { | ||
| return Type.INFORMATIVE; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| package analyzer.exercises.carsassemble; | ||
|
|
||
| import analyzer.Analyzer; | ||
| import analyzer.OutputCollector; | ||
| import analyzer.Solution; | ||
| import analyzer.comments.ExemplarSolution; | ||
| import analyzer.comments.MethodTooLong; | ||
| import analyzer.comments.ReuseCode; | ||
| import com.github.javaparser.ast.body.MethodDeclaration; | ||
| import com.github.javaparser.ast.expr.MethodCallExpr; | ||
| import com.github.javaparser.ast.stmt.BlockStmt; | ||
| import com.github.javaparser.ast.stmt.IfStmt; | ||
| import com.github.javaparser.ast.stmt.Statement; | ||
| import com.github.javaparser.ast.visitor.VoidVisitorAdapter; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| /** | ||
| * The {@link CarsAssembleAnalyzer} is the analyzer implementation for the {@code cars-assemble} concept exercise. | ||
| * | ||
| * @see <a href="https://github.com/exercism/java/tree/main/exercises/concept/cars-assemble">The cars-assemble exercise on the Java track</a> | ||
| */ | ||
| public class CarsAssembleAnalyzer extends VoidVisitorAdapter<OutputCollector> implements Analyzer { | ||
| private static final String EXERCISE_NAME = "CarsAssemble"; | ||
| private static final String MAGIC_NUMBER = "221"; | ||
| private static final String PRODUCTION_RATE_PER_HOUR_METHOD = "productionRatePerHour"; | ||
| private static final String WORKING_ITEMS_PER_MINUTE_METHOD = "workingItemsPerMinute"; | ||
| private static final String RETURN = "return"; | ||
|
|
||
| @Override | ||
| public void analyze(Solution solution, OutputCollector output) { | ||
|
|
||
| for (var compilationUnit : solution.getCompilationUnits()) { | ||
| compilationUnit.getClassByName(EXERCISE_NAME).ifPresent(c -> c.accept(this, output)); | ||
| } | ||
|
|
||
| if (output.getComments().isEmpty()) { | ||
| output.addComment(new ExemplarSolution(EXERCISE_NAME)); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void visit(MethodDeclaration n, OutputCollector output) { | ||
|
|
||
| if(n.getNameAsString().equals(PRODUCTION_RATE_PER_HOUR_METHOD) && !hasHelperMethod(n)){ | ||
| output.addComment(new MethodTooLong(PRODUCTION_RATE_PER_HOUR_METHOD)); | ||
| } | ||
|
|
||
| if(n.getNameAsString().equals(WORKING_ITEMS_PER_MINUTE_METHOD) && !reuseMethod(n)){ | ||
| output.addComment(new ReuseCode(WORKING_ITEMS_PER_MINUTE_METHOD, PRODUCTION_RATE_PER_HOUR_METHOD)); | ||
| } | ||
|
|
||
| if(useMagicNumber(n)){ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method seems to check if the number
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the value appears only once, but there is no final variable declared within the method? Something like: public double productionRatePerHour(int speed) {
return 221 * speed * successRate(speed);
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current version correctly flags magic numbers. Since this method ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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);
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense! Apologies, I completely missed this angle. Indeed, this is not a magic number situation!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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;
}There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 To be honest, I thought this solution looked ok (assuming this is the only place where public double productionRatePerHour(int speed) {
return 221 * speed * successRate(speed);
}If |
||
| output.addComment(new PreferStoringConstantInField()); | ||
| } | ||
|
|
||
| super.visit(n, output); | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| public void visit(IfStmt node, OutputCollector output){ | ||
|
|
||
| if(node.getThenStmt().toString().contains(RETURN) && node.hasElseBlock()){ | ||
| output.addComment(new AvoidUsingReturnInElseStatement()); | ||
| } | ||
|
|
||
| super.visit(node, output); | ||
| } | ||
|
|
||
| private boolean hasHelperMethod(MethodDeclaration n){ | ||
|
|
||
| if(n.getBody().isEmpty()){ | ||
| return true; | ||
| } | ||
|
|
||
| BlockStmt stmt = n.getBody().get(); | ||
| List<Statement> IfStmts = stmt.getStatements().stream().filter(Statement::isIfStmt).toList(); | ||
| for(Statement s : IfStmts){ | ||
| return ((IfStmt) s).hasElseBlock(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something here seems strange here. You get a list of Even if you really did mean to check just the first 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;
}
} |
||
| } | ||
| return true; | ||
| } | ||
|
|
||
| private boolean reuseMethod(MethodDeclaration n){ | ||
| return !n.findAll(MethodCallExpr.class).stream().filter(m -> m.getNameAsString().equals(PRODUCTION_RATE_PER_HOUR_METHOD)).toList().isEmpty(); | ||
| } | ||
|
|
||
| private boolean useMagicNumber(MethodDeclaration n){ | ||
|
|
||
| if(n.getBody().isEmpty()){ | ||
| return false; | ||
| } | ||
|
|
||
| BlockStmt stmt = n.getBody().get(); | ||
| return stmt.toString().contains(MAGIC_NUMBER); | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package analyzer.exercises.carsassemble; | ||
|
|
||
| import analyzer.Comment; | ||
|
|
||
| /** | ||
| * @see <a href="https://github.com/exercism/website-copy/blob/main/analyzer-comments/java/cars-assemble/prefer_storing_constant_in_field.md">Markdown Template</a> | ||
| */ | ||
| public class PreferStoringConstantInField extends Comment { | ||
| @Override | ||
| public String getKey() { | ||
| return "java.cars-assemble.prefer_storing_constant_in_field"; | ||
| } | ||
|
|
||
| @Override | ||
| public Type getType() { | ||
| return Type.INFORMATIVE; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "comments": [ | ||
| { | ||
| "comment": "java.general.exemplar", | ||
| "params": { | ||
| "exerciseName": "CarsAssemble" | ||
| }, | ||
| "type": "celebratory" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| { | ||
| "comments": [ | ||
| { | ||
| "comment": "java.general.reuse_code", | ||
| "params": { | ||
| "callingMethod": "workingItemsPerMinute", | ||
| "methodToCall": "productionRatePerHour" | ||
| }, | ||
| "type": "actionable" | ||
| }, | ||
| { | ||
| "comment": "java.general.feedback_request", | ||
| "params": {}, | ||
| "type": "informative" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| { | ||
| "comments": [ | ||
| { | ||
| "comment": "java.general.method_too_long", | ||
| "params": { | ||
| "methodNames": "productionRatePerHour" | ||
| }, | ||
| "type": "actionable" | ||
| }, | ||
| { | ||
| "comment": "java.general.feedback_request", | ||
| "params": {}, | ||
| "type": "informative" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "comments": [ | ||
| { | ||
| "comment": "java.cars-assemble.prefer_storing_constant_in_field", | ||
| "params": {}, | ||
| "type": "informative" | ||
| }, | ||
| { | ||
| "comment": "java.general.feedback_request", | ||
| "params": {}, | ||
| "type": "informative" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "comments": [ | ||
| { | ||
| "comment": "java.cars-assemble.avoid_using_return_in_else_statement", | ||
| "params": {}, | ||
| "type": "informative" | ||
| }, | ||
| { | ||
| "comment": "java.general.feedback_request", | ||
| "params": {}, | ||
| "type": "informative" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
|
|
||
| public class CarsAssemble { | ||
|
|
||
| private final int defaultProductionRate = 221; | ||
|
|
||
| public double productionRatePerHour(int speed) { | ||
| return defaultProductionRate * speed * successRate(speed); | ||
| } | ||
|
|
||
| public int workingItemsPerMinute(int speed) { | ||
| return (int) (productionRatePerHour(speed) / 60); | ||
| } | ||
|
|
||
| private double successRate(int speed) { | ||
| if (speed == 10) { | ||
| return 0.77; | ||
| } | ||
|
|
||
| if (speed == 9) { | ||
| return 0.8; | ||
| } | ||
|
|
||
| if (speed >= 5) { | ||
| return 0.9; | ||
| } | ||
|
|
||
| return 1; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
|
|
||
| public class CarsAssemble { | ||
|
|
||
| private final int defaultProductionRate = 221; | ||
|
|
||
| public double productionRatePerHour(int speed) { | ||
| return defaultProductionRate * speed * successRate(speed); | ||
| } | ||
|
|
||
| public int workingItemsPerMinute(int speed) { | ||
| return (int) defaultProductionRate * speed * successRate(speed) / 60; | ||
| } | ||
|
|
||
| private double successRate(int speed) { | ||
| if (speed == 10) { | ||
| return 0.77; | ||
| } | ||
|
|
||
| if (speed == 9) { | ||
| return 0.8; | ||
| } | ||
|
|
||
| if (speed >= 5) { | ||
| return 0.9; | ||
| } | ||
|
|
||
| return 1; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
|
|
||
| public class CarsAssemble { | ||
|
|
||
| private final int defaultProductionRate = 221; | ||
|
|
||
| public double productionRatePerHour(int speed) { | ||
| double rate = null; | ||
|
|
||
| if (speed == 10) { | ||
| rate = 0.77; | ||
| }else if (speed == 9) { | ||
| rate = 0.8; | ||
| } else if (speed >= 5) { | ||
| rate = 0.9; | ||
| } else { | ||
| rate = 1.0; | ||
| } | ||
|
|
||
| return defaultProductionRate * speed * rate; | ||
| } | ||
|
|
||
| public int workingItemsPerMinute(int speed) { | ||
| return (int) productionRatePerHour(speed) / 60; | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
|
|
||
| public class CarsAssemble { | ||
|
|
||
| public double productionRatePerHour(int speed) { | ||
| return 221 * speed * successRate(speed); | ||
| } | ||
|
|
||
| public int workingItemsPerMinute(int speed) { | ||
| return (int) productionRatePerHour(speed) / 60; | ||
| } | ||
|
|
||
| private double successRate(int speed) { | ||
| if (speed == 10) { | ||
| return 0.77; | ||
| } | ||
|
|
||
| if (speed == 9) { | ||
| return 0.8; | ||
| } | ||
|
|
||
| if (speed >= 5) { | ||
| return 0.9; | ||
| } | ||
|
|
||
| return 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that reusing the
MethodTooLongcould be confusing for the students. There's nothing in the comment that hints students should move theifstatement. 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 theifstatement into a separate method. The actionable level may be too high for this comment too.