-
Notifications
You must be signed in to change notification settings - Fork 39
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
chore: simplifying workflow samples #1716
chore: simplifying workflow samples #1716
Conversation
@@ -36,11 +33,10 @@ public Effect<String> create(@PathVariable String id, @PathVariable int initBala | |||
|
|||
@PatchMapping("/withdraw/{amount}") // <2> | |||
public Effect<String> withdraw(@PathVariable int amount) { | |||
var newBalance = currentState().balance() - amount; | |||
if (newBalance < 0) { | |||
Wallet updatedWallet = currentState().withdraw(amount); |
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.
Yeah, why not, just please add similar changes to java-spring-transfer-workflow-compensation
Watch out when merging this to not have 'simplifying?' as the commit message. Better to always start with a descriptive name for the first commit and PR title to avoid accidentally adding something meanless to the commit history. |
@@ -58,7 +54,7 @@ public Effect<String> deposit(@PathVariable int amount) { | |||
} | |||
|
|||
@GetMapping // <4> | |||
public Effect<Balance> get() { | |||
public Effect<Integer> get() { |
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.
It's better to return a complex type.
I think that returning Interge will have a similar effect as returning a String (see #1686).
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.
Good point. Although, I don't think an Integer would compose as a String but I don't know how to test it. How did you?
On the other hand, if it does compose like a String I think we could add a comment somewhere in the docs.
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.
you are right, it will just be a raw number being returned with content type json
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.
Cool. We agree then 👍
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
WDYT? @aludwiko