Added LCM with test case #17
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 12
Lines 492 503 +11
=====================================
+ Hits 492 503 +11
Continue to review full report at Codecov.
|
|
Hi @faheel, Can you please point where is this check failing? |
include/functions/math.hpp
Outdated
|
|
||
| // Trivial Case handling | ||
| if(abs_num1 == 0 && abs_num2 == 0) | ||
| throw std::logic_error("LCM undefined: Both number are zero"); |
| -------------------- | ||
| */ | ||
|
|
||
| BigInt lcm(const BigInt &num1, const BigInt &num2){ |
There was a problem hiding this comment.
The lcm() function can be written more simply as just this line:
return abs(num1 * num2) / gcd(num1, num2);There was a problem hiding this comment.
@faheel
I changed as mentioned. Please review once. I wrote LCM function that way because it is space efficient than current.
There was a problem hiding this comment.
The single line above is the most efficient as there are no conditions being checked, and has the least number of temporary variables.
There was a problem hiding this comment.
@faheel
Ok thanks.
Do I need to do something more in this. I have updated this as asked.
test/functions/math.test.cpp
Outdated
| long long naive_lcm(long long a, long long b){ | ||
| a = abs(a); | ||
| b = abs(b); | ||
|
|
There was a problem hiding this comment.
This function also needs to be simplified.
include/functions/math.hpp
Outdated
|
|
||
| BigInt lcm(const BigInt &num1, const BigInt &num2){ | ||
| // Trivial Case handling | ||
| if(abs_num1 == 0 || abs_num2 == 0) return 0; |
There was a problem hiding this comment.
if both are zero then gcd is zero and removing it gives division by zero error.
There was a problem hiding this comment.
Ah, yes, I missed this point. But don't forget to remove the abs_ prefix or it won't compile.
include/functions/math.hpp
Outdated
| // Trivial Case handling | ||
| if(abs_num1 == 0 || abs_num2 == 0) return 0; | ||
|
|
||
| return abs(num1 * num2)/gcd(abs_num1, abs_num2); |
|
@faheel |
|
Hi @faheel , Thanks |
|
Sorry for the late review, I was a little busy. Thank you for contributing! 🙂 |
Added definition for LCM in math.hpp and test cases. Having confusion in LCM(0, a) assuming it as 0. Please review.