doc(spanner): update sample implementation to match description #15871
Conversation
product-auto-label
bot
added
api: spanner
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the ReadWriteTransaction sample to better match its description by adding a check for sufficient funds before performing a transfer. The changes correctly implement the described logic and add specific error handling for the insufficient funds case. My review includes one suggestion to improve maintainability by replacing a duplicated hardcoded error message with a named constant, which aligns with the repository's C++ style guide.
| if (!commit && commit.status().code() == StatusCode::kFailedPrecondition && | ||
| commit.status().message() == "Album budget funds are insufficient") { |
There was a problem hiding this comment.
Comparing against a hardcoded error message string is brittle and violates the Google C++ Style Guide's rule against magic strings. This string literal is used in two places (here and in the transaction lambda on line 3458). If one of them is changed, the logic will break.
To make this more robust, please define the error message as a constant and use it in both places.
For example:
// At the top of ReadWriteTransaction
char const* const kInsufficientFundsMessage = "Album budget funds are insufficient";
// ... then in the lambda ...
if (*b2 < transfer_amount) {
return Status(StatusCode::kFailedPrecondition, kInsufficientFundsMessage);
}
// ... and here ...
if (!commit && commit.status().code() == StatusCode::kFailedPrecondition &&
commit.status().message() == kInsufficientFundsMessage) {
// ...
}References
- The Google C++ Style Guide, which this repository follows, recommends defining literal values like strings as named constants to improve readability and maintainability, especially when they are used in multiple places or for logical comparisons. (link)
There was a problem hiding this comment.
Just curious, do we not follow this convention for error messages?
There was a problem hiding this comment.
Just omit this part and let the function throw the status like any other error (rather than gobbling it).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15871 +/- ##
=======================================
Coverage 92.94% 92.94%
=======================================
Files 2458 2458
Lines 227590 227594 +4
=======================================
+ Hits 211527 211544 +17
+ Misses 16063 16050 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (!commit && commit.status().code() == StatusCode::kFailedPrecondition && | ||
| commit.status().message() == "Album budget funds are insufficient") { |
There was a problem hiding this comment.
Just omit this part and let the function throw the status like any other error (rather than gobbling it).
| return Status(StatusCode::kFailedPrecondition, | ||
| "Album budget funds are insufficient"); |
There was a problem hiding this comment.
Take a look at the other language samples (https://docs.cloud.google.com/spanner/docs/samples/spanner-read-write-transaction). If they check this, the most common message is "The second album does not have enough funds to transfer". C++ might as well do the same.
3 participants
Sample description in question:
Your marketing department asks you to move $200,000 from the budget of Albums (2, 2) to Albums (1, 1), but only if the money is available in that album's budget. You should use a locking read-write transaction for this operation, because the transaction might perform writes depending on the result of a read.