exercism / java-analyzer

GNU Affero General Public License v3.0
7 stars 14 forks source link

Log Levels: feedback incorrectly recommends to use the substring method #150

Closed santhoshswamyv closed 3 months ago

santhoshswamyv commented 4 months ago

Issue in Log Levels Exercise

I used the substring method to solve the exercise but the automated feedback insists me to use substring. Resolve this issue as soon as possible.

image

class LogLevels {

    public static String message(String logLine) {
        logLine = logLine.substring(logLine.indexOf(":") + 1);
        return logLine.trim();
    }

    public static String logLevel(String logLine) {
        if (logLine.toLowerCase().contains("info"))
            return "info";
        if (logLine.toLowerCase().contains("warning"))
            return "warning";

        return "error";
    }

    public static String reformat(String logLine) {
        return message(logLine) + " (" + logLevel(logLine) + ")";
    }
}
sanderploegsma commented 4 months ago

Thanks for your feedback! This seems like an interesting bug in the analyzer, we'll look into it.

Could you please add a comment to this issue with your solution code as text instead of as a screenshot? This makes it easier for our contributing team to work on this issue, as they can use it to test whether the bug has been solved. And to make it readable, I'd suggest using code blocks when you do.

Resolve this issue as soon as possible.

Please be mindful in the way you communicate to others. You may not have intended it this way, but the tone of this comment is very demanding and could even be considered as disrespectful to the Exercism contributors, who build this platform as volunteers and in their spare time.

sanderploegsma commented 4 months ago

@manumafe98 any idea?

The analyzer feedback was probably triggered by the fact that the logLevel method doesn't use substring, right? Based on the implementation though, I would have expected it to say something about the hard-coded log levels, but maybe that is only looking for upper-case values like "INFO", "ERROR" etc.

santhoshswamyv commented 4 months ago

Thanks for your feedback! This seems like an interesting bug in the analyzer, we'll look into it.

Could you please add a comment to this issue with your solution code as text instead of as a screenshot? This makes it easier for our contributing team to work on this issue, as they can use it to test whether the bug has been solved. And to make it readable, I'd suggest using code blocks when you do.

Resolve this issue as soon as possible.

Please be mindful in the way you communicate to others. You may not have intended it this way, but the tone of this comment is very demanding and could even be considered as disrespectful to the Exercism contributors, who build this platform as volunteers and in their spare time.

Iā€™m sorry if my previous message came across as demanding or disrespectful. That was not my intention. I appreciate the hard work you all put into this platform. Thank you for your understanding.

MrXcitement commented 4 months ago

I am also receiving the feedback of 'Recommended: Consider using the substring method to solve this exercise.' when submitting my solutions. I am using substring in the logLevel method, to return the text between left and right square braces. Could this be due to the fact in the messages method, I am not using substring, but split?

My last submitted solution:

public class LogLevels {

    public static String message(String logLine) {
        return logLine.split(": ", 2)[1]
        .trim();
    }

    public static String logLevel(String logLine) {
        return logLine
        .substring(logLine.indexOf('[') + 1, logLine.indexOf(']'))
        .toLowerCase();
    }

    public static String reformat(String logLine) {
        return message(logLine) + " (" + logLevel(logLine) + ")";
    }
}
sanderploegsma commented 4 months ago

@MrXcitement thanks for your feedback! I think you're right, you're receiving the analyzer feedback because the message method doesn't use substring.

@manumafe98 any particular reason why the analyzer should instruct students to choose substring over split? IMO the solution above is great and doesn't need further improvements, but perhaps you have a different opinion. If not, perhaps we should revisit the analyzer implementation to narrow down the cases where it suggests to use substring.

manumafe98 commented 4 months ago

@manumafe98 any particular reason why the analyzer should instruct students to choose substring over split? IMO the solution above is great and doesn't need further improvements, but perhaps you have a different opinion. If not, perhaps we should revisit the analyzer implementation to narrow down the cases where it suggests to use substring.

Well in the concept exercise we were guiding students to the exemplar solution right? And also we defined this comment on the design.md

Maybe we could also allow the usage of split in the message method

sanderploegsma commented 4 months ago

Yes, that is indeed the point. However, I'd argue that sometimes a solution can be equivalent to the exemplar solution without being the same. šŸ˜‰

In fact, given that this exercise teaches the strings concept, I like the solution shared by @MrXcitement since it applies the concept in multiple ways using both split and substring šŸ‘