bank-account: describe Analyzer feedback in .meta/design.md #2714#2946
bank-account: describe Analyzer feedback in .meta/design.md #2714#2946masiljangajji wants to merge 4 commits intoexercism:mainfrom
Conversation
kahgoh
left a comment
kahgoh
left a comment
There was a problem hiding this comment.
Thanks for looking into this and for the suggestions! My thoughts on this one in the comments below...
|
|
||
| This exercise could benefit from the following rules in the [analyzer]: | ||
|
|
||
| - `essential`: Verify that the solution **guards `deposit`, `withdraw`, and `getBalance` with `synchronized`** to avoid |
There was a problem hiding this comment.
I think the problem with checking that the methods are synchronized it that there are different ways of achieving the same thing with different trade offs. For example:
- Lock objects (
ReentrantLock/ReadWriteLock) (this solution uses locks) - Synchronized statements (using intrinsic locks)
The instructions also hint at this:
To pass the last test you might find the synchronized keyword or locks useful.
I think each method has their own strengths and weaknesses and there may be more I haven't thought of. We probably should check that they use at least one of these techniques to guard concurrent access, but I'm not sure how easy/hard that will be...
| - `essential`: Verify that the solution **checks the account’s open/closed state before every balance operation** | ||
| - `essential`: If the solution **allows negative amounts** in `deposit` or `withdraw`, instruct the student to reject | ||
| them early. | ||
| - `essential`: If the solution **does not throw an exception when `amount` exceeds `balance` during withdrawal**, | ||
| instruct the student to add this check. |
There was a problem hiding this comment.
Looking at the tests, these are already covered by the following test cases (so the analyzer doesn't need to check for them):
cannotWithdrawMoreThanWasDepositedcannotWithdrawNegativeAmountcannotDepositNegativeAmountcannotWithdrawMoreThanWasDeposited
| them early. | ||
| - `essential`: If the solution **does not throw an exception when `amount` exceeds `balance` during withdrawal**, | ||
| instruct the student to add this check. | ||
| - `actionable`: Verify that the solution **declares exactly one `boolean`/`Boolean` field to track account state**; if the count is zero or greater than one, instruct the student to use exactly one such field. |
There was a problem hiding this comment.
I'm somewhat unsure about this one. Most solutions do use a boolean to track whether an account is open or not, but there are also other ways. For example, I have seen one that use an AtomicBoolean and another that uses a string (although, I wouldn't recommend using a string...). Some also prefer Enums over booleans (like this solution)
2 participants
pull request
closes #2714
The goal is to maintain thread safety while throwing exceptions for any exceptional situations that may arise.
Reviewer Resources:
Track Policies