-
Notifications
You must be signed in to change notification settings - Fork 8
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
refactor: streamline arithmetic traits with macros #99
Conversation
Consolidated repetitive implementations of `Add`, `Sub`, `Mul`, and `Div` into reusable macros for `FractionLike`, reducing code duplication and improving maintainability. This change ensures uniform functionality across operations and simplifies future updates.
WalkthroughThe pull request introduces a macro-based approach to implement arithmetic operations for the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/entities/fractions/fraction.rs (2)
228-250
: Macro usage for addition and subtraction is concise and effective.By using macros, you eliminate repetitive code and ensure uniform behavior for both owned and borrowed types. Good job on implementing DRY principles here!
You might consider documenting the macro more thoroughly (e.g., how it handles denominators that are equal vs. not equal) so that future maintainers understand the generated code paths clearly.
253-257
: Potential consideration for fraction normalization.While these macros correctly sum or subtract fractions, the resulting fraction is not normalized (e.g., gcd-based reduction). This is typically fine unless there's a long chain of fractional operations. Consider optional fraction reduction to avoid ballooning numerators or denominators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/entities/fractions/fraction.rs
(1 hunks)
🔇 Additional comments (3)
src/entities/fractions/fraction.rs (3)
258-272
: Macro-based multiplication logic appears straightforward.The multiplication approach is correct: multiply numerators and denominators and keep the same metadata. It's consistent with the code style you’ve established.
275-277
: Check for tests with borrowed vs. owned operands.You have macros for both
Mul<Self>
andMul<&Self>
, but the current tests don't appear to explicitly cover the borrowed version. It might be worth ensuring you have direct tests or confirm that coverage is adequately provided via the existing test suite.
278-292
: Division implementation is consistent with fraction logic.The macro-based approach for division is also clean and mirrors your existing fraction logic. Good job maintaining consistency across arithmetic macros.
Consolidated repetitive implementations of
Add
,Sub
,Mul
, andDiv
into reusable macros forFractionLike
, reducing code duplication and improving maintainability. This change ensures uniform functionality across operations and simplifies future updates.Summary by CodeRabbit