logs-logs-logs: describe Analyzer feedback #2939
Conversation
There was a problem hiding this comment.
Thanks for taking this on @habbie321! I have taken a look and have added some comments. In general, the analyzer dosen't need to check for things that would be picked up by the tests or the compiler.
In addition, could you please add a comment to #2681 asking for the issue to be assigned to you? This is to allow me to assign the issue to you.
| - `essential`: Verify that a switch statement was used. If they used if-else, comment that switch is better due to there being many cases. | ||
| - `actionable`: The function should be public. | ||
| 2. Support unknown log level | ||
| - `essential`: Verify that an unknown constant is present in the enum declaration. |
There was a problem hiding this comment.
The analyzer doesn't need to check for this because it is covered by the tests.
| - `essential`: Verify that an unknown constant is present in the enum declaration. |
|
|
||
| 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. |
There was a problem hiding this comment.
The analyzer doesn't need to check for this because it is covered by the tests.
| - `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. | ||
| - `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. | ||
| - `actionable`: The function should be public. |
There was a problem hiding this comment.
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.
| - `actionable`: The function should be public. |
| - `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. |
There was a problem hiding this comment.
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.
| 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. | ||
| - `actionable`: The enum should be public; prompt the student to think of encapsulation. |
There was a problem hiding this comment.
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.
| - `actionable`: The enum should be public; prompt the student to think of encapsulation. |
| - `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. | ||
| - `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. |
There was a problem hiding this comment.
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`: 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. | ||
| - `informative`: Let them know they can write it in one line. |
There was a problem hiding this comment.
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"?
| - `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. |
There was a problem hiding this comment.
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.
pull request
Please give feedback, this is my first time. Resolves #2681
Reviewer Resources:
Track Policies