-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Implement analyzer for secrets
#141
Implement analyzer for secrets
#141
Conversation
secrets
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.
Approving via hand waving :) Someone else should do the actual Java code review :)
src/main/java/analyzer/exercises/secrets/ImplementOperator.java
Outdated
Show resolved
Hide resolved
I found another case that we may be able to comment on: public class Secrets {
public static int shiftBack(int value, int amount) {
if (value<0) return value>>>amount;
return value>>amount;
}
} Perhaps we can add an |
Instead of being that specific I think I'm going to make a comment to avoid any conditional logic, so they focus only on using the correct operator |
Udating Analyzer root to run the analyzer on alphabetical order Changing implement operator comment to use bitwise operator Updating the analyzer to only return 1 essential comment at a time Generalizing the method that checks the usage of the operator Adding extra comment to check for conditional logic Updating preferBitwiseNot to trigger if the student used bitwise and
@sanderploegsma I should add extra smoke tests? or we should limit to create 2/3? |
The current amount is fine. The primary goal of these tests is to test the analyzer end-to-end just to make sure we didn't make a mistake that would otherwise be missed because it's not covered by the JUnit tests. Basically we want to see that the analyzer provides some output for each exercise. It's not needed to cover all scenarios this way, that's what the JUnit tests are for. |
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.
LGTM 👍
closes #112
Todo: