Skip to content

Implementation of analyzer for wizards-and-warriors-2 #262

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/analyzer/AnalyzerRoot.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import analyzer.exercises.secrets.SecretsAnalyzer;
import analyzer.exercises.twofer.TwoferAnalyzer;
import analyzer.exercises.wizardsandwarriors.WizardsAndWarriorsAnalyzer;
import analyzer.exercises.wizardsandwarriors2.WizardsAndWarriors2Analyzer;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -33,7 +34,6 @@ private AnalyzerRoot() {
*/
public static Output analyze(Solution solution) {
var outputBuilder = new OutputBuilder();

for (Analyzer analyzer : createAnalyzers(solution.getSlug())) {
analyzer.analyze(solution, outputBuilder);
}
Expand All @@ -49,7 +49,6 @@ private static List<Analyzer> createAnalyzers(String slug) {
var analyzers = new ArrayList<Analyzer>();

analyzers.add(new GlobalAnalyzer());

switch (slug) {
case "annalyns-infiltration" -> analyzers.add(new AnnalynsInfiltrationAnalyzer());
case "hamming" -> analyzers.add(new HammingAnalyzer());
Expand All @@ -61,6 +60,7 @@ private static List<Analyzer> createAnalyzers(String slug) {
case "secrets" -> analyzers.add(new SecretsAnalyzer());
case "two-fer" -> analyzers.add(new TwoferAnalyzer());
case "wizards-and-warriors" -> analyzers.add(new WizardsAndWarriorsAnalyzer());
case "wizards-and-warriors-2" -> analyzers.add(new WizardsAndWarriors2Analyzer());
}

return List.copyOf(analyzers);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package analyzer.exercises.wizardsandwarriors2;

import analyzer.Comment;

/**
* @author: chiarazarrella
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to tell students to use method overloading - the exercise has been designed so that the GameMaster.describe must be overloaded. So, all passing solutions should already be using method overload to pass.

public class UseMethodOverloading extends Comment{

@Override
public String getKey() {
return "java.wizards-and-warriors-2.use_method_overloading";
}

@Override
public Type getType() {
return Type.ACTIONABLE;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package analyzer.exercises.wizardsandwarriors2;

import analyzer.Analyzer;
import analyzer.OutputCollector;
import analyzer.Solution;
import analyzer.comments.ExemplarSolution;
import analyzer.comments.PreferStringConcatenation;
import com.github.javaparser.ast.body.MethodDeclaration;
import com.github.javaparser.ast.expr.MethodCallExpr;
import com.github.javaparser.ast.visitor.VoidVisitorAdapter;

import java.util.List;

/**
* The {@link WizardsAndWarriors2Analyzer} is the analyzer implementation for the {@code wizards-and-warriors-2} concept exercise.
*
* @see <a href="https://github.com/exercism/java/tree/main/exercises/concept/wizards-and-warriors-2">The wizards-and-warriors exercise on the Java track</a>
*/
public class WizardsAndWarriors2Analyzer extends VoidVisitorAdapter<OutputCollector> implements Analyzer {

private static final String EXERCISE_NAME = "Wizards and Warriors 2";
private static final String GAME_MASTER = "GameMaster";
private static final String DESCRIBE = "describe";
private static final String FORMAT = "format";

@Override
public void analyze(Solution solution, OutputCollector output) {

for (var compilationUnit : solution.getCompilationUnits()) {
compilationUnit.getClassByName(GAME_MASTER).ifPresent(c -> c.accept(this, output));
}

if (output.getComments().isEmpty()) {
output.addComment(new ExemplarSolution(EXERCISE_NAME));
}
}

@Override
public void visit(MethodDeclaration node, OutputCollector output) {

if(!node.getNameAsString().equals(DESCRIBE)) {
return;
}

if(node.getParameters().size() > 1 && !useOverload(node)) {

output.addComment(new UseMethodOverloading());

}

if(useStringFormat(node)) {

output.addComment(new PreferStringConcatenation());

}


super.visit(node, output);
}

private static boolean useOverload(MethodDeclaration node) {

int paramCount = node.getParameters().size();

List<MethodCallExpr> describeCalls = node.findAll(MethodCallExpr.class).stream()
.filter(m -> m.getNameAsString().equals(DESCRIBE))
.toList();

if (paramCount == 2) {
return describeCalls.size() == 1 || describeCalls.size() == 3;
}

if (paramCount == 3) {
return describeCalls.size() == 3;
}

return false;
}

private static boolean useStringFormat(MethodDeclaration node) {
return node.findAll(MethodCallExpr.class).stream()
.anyMatch(m -> m.getNameAsString().contains(FORMAT));
}

}
16 changes: 16 additions & 0 deletions src/test/java/analyzer/AnalyzerIntegrationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -205,4 +205,20 @@ void salarycalculator(String scenario) throws IOException {

Approvals.verify(serialize(output.analysis()), Approvals.NAMES.withParameters(scenario));
}

@ParameterizedTest
@ValueSource(strings = {
"ExemplarSolution",
"NotUseMethodOverloading",
"PartialUseOfMethodOverloading",
"UseStringFormat",
"UseStringFormatAndNotUseMethodOverloading",
})
void wizardsandwarriors2(String scenario) throws IOException {
var path = Path.of("wizards-and-warriors-2", scenario + ".java");
var solution = new SolutionFromFiles("wizards-and-warriors-2", SCENARIOS.resolve(path));
var output = AnalyzerRoot.analyze(solution);

Approvals.verify(serialize(output.analysis()), Approvals.NAMES.withParameters(scenario));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"comments": [
{
"comment": "java.general.exemplar",
"params": {
"exerciseName": "Wizards and Warriors 2"
},
"type": "celebratory"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"comments": [
{
"comment": "java.wizards-and-warriors-2.use_method_overloading",
"params": {},
"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.wizards-and-warriors-2.use_method_overloading",
"params": {},
"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.general.prefer_string_concatenation",
"params": {},
"type": "informative"
},
{
"comment": "java.general.feedback_request",
"params": {},
"type": "informative"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"comments": [
{
"comment": "java.wizards-and-warriors-2.use_method_overloading",
"params": {},
"type": "actionable"
},
{
"comment": "java.general.prefer_string_concatenation",
"params": {},
"type": "informative"
},
{
"comment": "java.general.feedback_request",
"params": {},
"type": "informative"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
public class GameMaster {

public String describe(Character character) {
return "You're a level " + character.getLevel() + " " + character.getCharacterClass() + " with " + character.getHitPoints() + " hit points.";
}

public String describe(Destination destination) {
return "You've arrived at " + destination.getName() + ", which has " + destination.getInhabitants() + " inhabitants.";
}

public String describe(TravelMethod travelMethod) {
if (travelMethod == TravelMethod.WALKING) {
return "You're traveling to your destination by walking.";
}
return "You're traveling to your destination on horseback.";
}

public String describe(Character character, Destination destination, TravelMethod travelMethod) {
return describe(character) + " " + describe(travelMethod) + " " + describe(destination);
}

public String describe(Character character, Destination destination) {
return describe(character, destination, TravelMethod.WALKING);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at your tests for the "not use", "partial" and "use string format not use" method overloading, I'm a little confused into how method overloading isn't being used in their samples because the describe method is overloaded (defined multiple times, with different parameter types). Did you mean reuse method?

Copy link
Author

@chiarazarrella chiarazarrella Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I see; what I meant was to exploit method overloading and yes, it is done indeed by reusing the methods.

However, I have noticed that it already exists something similar, the reuse_code comment. Should this be used instead? Or is it enough to only modify the name of the comment from use_method_overloading to reuse_method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, if we can, we should use the same reuse_code comment. The comment needs to be specific about which describe method it should use though because all the methods are named describe. I'd suggest including the parameter's type in the comment. For example:

The describe(Character, Destination) method should reuse the logic implemented in describe(Character, Destination, TravelMethod). Reusing existing methods can help make code easier to maintain.

The methods can be passed as parameters to the comment (see AnalyzerIntegrationTest.loglevels.NoReuseOfBothMethods.approved.txt for an example).

Copy link
Author

@chiarazarrella chiarazarrella Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I have a little doubt about the following, from the design.md:
If the student does not reuse the method describe(Character), describe(Destination), describe(TravelMethod) or describe(Character, Destination, TravelMethod) to solve the describe(Character, Destination) method, instruct them to do so.

Should we provide two comments? or could we modify the comment to inject also a sentence? Because I have seen that the reuse_code comment takes as input a string, we could manage to create a comment like this:

The describe(Character, Destination) method should reuse the logic implemented in describe(Character, Destination, TravelMethod) or in the individual methods describe(Character), describe(Destination), describe(TravelMethod). Reusing existing methods can help make code easier to maintain.

This means that the parameter acting as %<methodToCall>s will be a whole sentence:
describe(Character, Destination, TravelMethod) or in the single methods describe(Character), describe(Destination), describe(TravelMethod).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using two comments is fine and is the easier option. If we change the parameter to allow sentences as a parameter, we should check that the comments made in still make sense (because the parameter is enclosed in back ticks in the markdown).

The other possibility is to use a generic comment that can cover all the cases. For example:

The describe(Character, Destination) method should reuse the logic implemented in describe(Character, Destination, TravelMethod), describe(Character), describe(Destination) or describe(TravelMethod). Reusing existing methods can help make code easier to maintain.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should handle the backticks even with two comments, though. Especially, if we need to keep the highlight of the methods only.

For example, with the following overloaded method describe(Character, Destination, TravelMethod), the comment would be:

The describe(Character, Destination, TravelMethod) method should reuse the logic implemented in describe(Character), describe(Destination) and describe(TravelMethod). Reusing existing methods can help make code easier to maintain.

and the injected string would be with the handle of the backticks:

describe(Character)`, `describe(Destination)`, and `describe(TravelMethod)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see what you mean! I think that could work, but I'd lean towards making a copy of the comment specifically for this case. For example, the entire comment would be (no parameters in the comment):

The `describe(Character, Destination, TravelMethod)` method should reuse the logic implemented in `describe(Character)`, `describe(Destination)` and `describe(TravelMethod)`. Reusing existing methods can help make code easier to maintain.

If we close and re-open back ticks in the parameters and someone make changes to the comment's format in the future, they will need to be aware of the usage here. For example, if the change was to make methods italic, like describe(Character), the comment would look like this:

The _`%<callingMethod>s`_ method should reuse the logic implemented in _`%<methodToCall>s`_.
Reusing existing methods can help make code easier to maintain.

We'd need to remember to update the parameter accordingly, otherwise the commas and the word and would also be in italics.

describe(Character)`_, _`describe(Destination)`_, and _`describe(TravelMethod)

If we make a copy of the comments with the hard coded methods, we still need to remember to update the copy. But, if we forget to update it, at least the formatting wouldn't look weird for the comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I agree on that!
Then, I will create these two specific comments and update you asap when everything is done. :)

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
public class GameMaster {

public String describe(Character character) {
return "You're a level " + character.getLevel() + " " + character.getCharacterClass() + " with " + character.getHitPoints() + " hit points.";
}

public String describe(Destination destination) {
return "You've arrived at " + destination.getName() + ", which has " + destination.getInhabitants() + " inhabitants.";
}

public String describe(TravelMethod travelMethod) {
if (travelMethod == TravelMethod.WALKING) {
return "You're traveling to your destination by walking.";
}
return "You're traveling to your destination on horseback.";
}

public String describe(Character character, Destination destination, TravelMethod travelMethod) {
return "You're a level " + character.getLevel() + " " + character.getCharacterClass() + " with " + character.getHitPoints() + " hit points. " +
(travelMethod == TravelMethod.WALKING ? "You're traveling to your destination by walking. " : "You're traveling to your destination on horseback. ") +
"You've arrived at " + destination.getName() + ", which has " + destination.getInhabitants() + " inhabitants.";
}

public String describe(Character character, Destination destination) {
return "You're a level " + character.getLevel() + " " + character.getCharacterClass() + " with " + character.getHitPoints() + " hit points. " +
"You're traveling to your destination by walking. " +
"You've arrived at " + destination.getName() + ", which has " + destination.getInhabitants() + " inhabitants.";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
public class GameMaster {

public String describe(Character character) {
return "You're a level " + character.getLevel() + " " + character.getCharacterClass() + " with " + character.getHitPoints() + " hit points.";
}

public String describe(Destination destination) {
return "You've arrived at " + destination.getName() + ", which has " + destination.getInhabitants() + " inhabitants.";
}

public String describe(TravelMethod travelMethod) {
if (travelMethod == TravelMethod.WALKING) {
return "You're traveling to your destination by walking.";
}
return "You're traveling to your destination on horseback.";
}

public String describe(Character character, Destination destination, TravelMethod travelMethod) {
return describe(character) + " " + describe(travelMethod) + " " + describe(destination);
}

public String describe(Character character, Destination destination) {
return describe(character) + " " + describe(destination) + " You're traveling to your destination by walking.";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
public class GameMaster {

public String describe(Character character) {
return "You're a level %d %s with %d hit points.".formatted(character.getLevel(),
character.getCharacterClass(), character.getHitPoints());
}

public String describe(Destination destination) {
return "You've arrived at %s, which has %d inhabitants.".formatted(destination.getName(),
destination.getInhabitants());
}

public String describe(TravelMethod travelMethod) {
if (travelMethod == TravelMethod.WALKING) {
return "You're traveling to your destination by walking.";
}
return "You're traveling to your destination on horseback.";

}

public String describe(Character character, Destination destination, TravelMethod travelMethod) {
return "%s %s %s".formatted(describe(character), describe(travelMethod), describe(destination));
}

public String describe(Character character, Destination destination) {
return describe(character, destination, TravelMethod.WALKING);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
public class GameMaster {

public String describe(Character character) {
return "You're a level %d %s with %d hit points.".formatted(character.getLevel(),
character.getCharacterClass(), character.getHitPoints());
}

public String describe(Destination destination) {
return "You've arrived at %s, which has %d inhabitants.".formatted(destination.getName(),
destination.getInhabitants());
}

public String describe(TravelMethod travelMethod) {
if (travelMethod == TravelMethod.WALKING) {
return "You're traveling to your destination by walking.";
}
return "You're traveling to your destination on horseback.";

}

public String describe(Character character, Destination destination, TravelMethod travelMethod) {
return "You're a level %d %s with %d hit points. ".formatted(character.getLevel(),
character.getCharacterClass(), character.getHitPoints()) +
(travelMethod == TravelMethod.WALKING ? "You're traveling to your destination by walking. "
: "You're traveling to your destination on horseback. ") +
"You've arrived at %s, which has %d inhabitants.".formatted(destination.getName(),
destination.getInhabitants());
}

public String describe(Character character, Destination destination) {
return "You're a level %d %s with %d hit points. You're traveling to your destination by walking. You've arrived at %s, which has %d inhabitants."
.formatted(character.getLevel(), character.getCharacterClass(), character.getHitPoints(),
destination.getName(), destination.getInhabitants());
}
}
Loading