Skip to content

Conversation

@manumafe98
Copy link
Contributor

@manumafe98 manumafe98 commented Mar 5, 2024

closes #112

Todo:

@manumafe98 manumafe98 added the x:size/medium Medium amount of work label Mar 5, 2024
@manumafe98 manumafe98 self-assigned this Mar 5, 2024
@manumafe98 manumafe98 requested a review from a team as a code owner March 5, 2024 00:09
@manumafe98 manumafe98 changed the title Implement analyzer for secrets Implement analyzer for secrets Mar 5, 2024
Copy link
Member

@ErikSchierboom ErikSchierboom left a 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 :)

@sanderploegsma
Copy link
Contributor

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 actionable comment on this, saying that using >>> works for both positive and negative values and no if-statement is required.

@manumafe98
Copy link
Contributor Author

manumafe98 commented Mar 5, 2024

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 actionable comment on this, saying that using >>> works for both positive and negative values and no if-statement is required.

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
@manumafe98
Copy link
Contributor Author

@sanderploegsma I should add extra smoke tests? or we should limit to create 2/3?

@sanderploegsma
Copy link
Contributor

sanderploegsma commented Mar 7, 2024

@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.

@manumafe98 manumafe98 requested review from sanderploegsma and removed request for sanderploegsma March 11, 2024 14:41
Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:size/medium Medium amount of work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

secrets: implement analyzer

3 participants