Skip to content

logs-logs-logs: describe Analyzer feedback #2939

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.

Sign up for GitHub

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 6 commits into
base: main
Choose a base branch
from
Open

logs-logs-logs: describe Analyzer feedback #2939

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7f6ba2
started analyzer feedback description
habbie321 Apr 22, 2025
2a80bc9
done?
habbie321 Apr 23, 2025
008572d
done..?
habbie321 Apr 23, 2025
ff76098
Implemented feedback
habbie321 May 18, 2025
1a96c52
Merge branch 'main' into issue-2681
habbie321 May 18, 2025
44db848
Merge remote-tracking branch 'origin/issue-2681' into issue-2681
habbie321 May 18, 2025
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
24 changes: 24 additions & 0 deletions exercises/concept/logs-logs-logs/.meta/design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,27 @@ This exercise's prerequisites Concepts are:
- `strings`
- `switch-statement`
- `constructors`

## Analyzer

This exercise could benefit from the following rules in the [analyzer][analyzer]:

1. Parse log level
- `LogLevel`
- `essential`: Verify that necessary and sufficient constants are present in enum declaration (minus unknown, for now) and that the constructor is present.
Copy link
Member

Choose a reason for hiding this comment

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

The analyzer doesn't need to check for this because it is covered by the tests.

Suggested change
- `essential`: Verify that necessary and sufficient constants are present in enum declaration (minus unknown, for now) and that the constructor is present.

- `actionable`: The enum should be public; prompt the student to think of encapsulation.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any point in the analyzer checking if the enum is public. It is already public when they start the exercise. Even if the student changes it, I don't think it is important enough in this exercise to check for it. If the intent is to make sure they don't set it private, the solution and tests won't even compile.

Suggested change
- `actionable`: The enum should be public; prompt the student to think of encapsulation.

- `LogLine.getLogLevel()`
- `essential`: Verify that a switch statement was used. If they used if-else, comment that switch is better due to there being many cases.
Copy link
Member

Choose a reason for hiding this comment

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

Ok with suggesting switch statements or switch expressions, but I think should be actionable instead of essential because the exercise's focus is on enums, not switch.

- `actionable`: The function should be public.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any point in the analyzer checking this. The method is public stubs. Even if they change it, I don't think it is important enough for the analyzer to check for this. If the intent is to make sure they don't set it private, the solution and tests won't even compile.

Suggested change
- `actionable`: The function should be public.

2. Support unknown log level
- `essential`: Verify that an unknown constant is present in the enum declaration.
Copy link
Member

Choose a reason for hiding this comment

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

The analyzer doesn't need to check for this because it is covered by the tests.

Suggested change
- `essential`: Verify that an unknown constant is present in the enum declaration.

- `essential`: Verify that a default was used in a switch statement. Comment as previously stated in the case of an if-else.
3. Convert log line to short format
- `essential`: Verify that the constructor is appropriately updated.
Copy link
Member

Choose a reason for hiding this comment

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

Which constructor is this referring to? Do you mean check there is a LogLevel(int encodedLevel) constructor? If so and if we're going to check there is the enum has field, the analyzer doesn't need to check for this because it won't compile if the field is added without adding the constructor.

- `essential`: Verify that there exists a field dedicated to the encoded log level.
- `actionable`: Said field should be private and final (this will then require the use of a getter for the log level!).
- `LogLine.getOutputForShortLog()`
- `celebratory`: Celebrate if this was written in one line.
Copy link
Member

Choose a reason for hiding this comment

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

The celebratory comment should apply if no comments are raised (not that something was written on one line). See captain's log design.md for example.

- `informative`: Let them know they can write it in one line.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's the thinking on this one. I'm not sure if the the analyzer can specifically enforce that something is written on just one line. For example, what if someone writes the following?

return logLevel.getTypeLog() 
    + ":" 
    + logMessage;

The formatting may split over multiple lines and I think that can be fine. Rather than checking if it is one line, what else can the analyzer check for as a sign that it isn't "one line"?


[analyzer]: https://github.com/exercism/java-analyzer