Skip to content
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

Division operation #18

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

MahmoudRizk
Copy link
Contributor

@MahmoudRizk MahmoudRizk commented Mar 6, 2017

Adding the division operation.
Allows the division of two integer numbers of "Bigint" data type, returning quotient and remainder.

Copy link
Owner

@kasparsklavins kasparsklavins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix merge conflicts?

@kasparsklavins kasparsklavins changed the title Mahmoud rizk Division operation Mar 6, 2017
@MahmoudRizk
Copy link
Contributor Author

ok, i will fix it.

@ghost
Copy link

ghost commented Mar 9, 2017

divided by zero will get into infinite loop @MahmoudRizk

@MahmoudRizk
Copy link
Contributor Author

@titansnow , Thanks, i will fix it.

@ghost
Copy link

ghost commented Mar 11, 2017

@MahmoudRizk , I found not only zero could cause this. Any number divided by nonpositive will also get into infinite loop. See this

#include"bigint.h"
using namespace Dodecahedron;
int main()
{Bigint a=10;
 Bigint b=-2;
 a/b;
 return 0;}

May you forget to check if number is not positive?

…y is correct in case of signed numbers division'
@MahmoudRizk
Copy link
Contributor Author

Division by zero bug is fixed.
Division of signed numbers bug half fixed, quotient will always be calculated correctly while the remainder is not.
The reason for not calculating the remainder correctly is because of the subtraction is not working properly.
There is someone fixed the subtraction code in the issues, but i don't want to commit code wasn't written by me.
@titansnow @kasparsklavins

@ghost
Copy link

ghost commented Mar 11, 2017

:)

@kasparsklavins
Copy link
Owner

I dont like the current solution to dividing by zero.

Throwing an exception might be acceptable, so the behaviour is the same as with int type.

@MahmoudRizk
Copy link
Contributor Author

changed the way of error handling, you can check it. @kasparsklavins

@MahmoudRizk
Copy link
Contributor Author

I didn't notice that you are the one who made the subtraction fixing 😆 , forgive me if you felt insult for mentioning you by someone. @titansnow

}
catch(const char* msg){
std::cerr << msg << std::endl;
std::terminate();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an exception is thrown, is it user catchable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, user won't catch it. It is caught in the division function interface only and printed on screen as shown above.
It is a little tricky and i don't know how to handle the exception, so please can you clarify more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing the error is enough, you dont have to catch it.

If division by zero occurs, let the user deal with it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My solution is to make an inheriting class to std::runtime_error named arithmetic_error and throw it with "Divide by zero" when divided by zero just like Java does. Like this function for checking:

void check_divisor(Bigint const &b)
{
    if (b == 0)
        throw Bigint::arithmetic_error("Divide by zero");
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@titansnow I agree. That would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i will try it.

src/bigint.cpp Outdated
Bigint zero("0");

if(q==zero || q == 0){
//std::cout << "Error: Dividing by zero" << std::endl;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the commented out code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will fix it.

private:
std::vector<Bigint> divide(Bigint q); // returns quotient(index[0]) and remainder(index[1]).
public:
std::vector<Bigint> operator/(Bigint q); // interface for divide() function.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does division return a vector?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it returns both quotient and remainder.

Copy link
Owner

@kasparsklavins kasparsklavins Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

divide() can stay as is, but / operator should return just Bigint so syntax like this would work a * b / c.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there can be both operator/ and operator% wrapper divide and split the results, return what is needed, just like killing two birds with one stone. By the way, I think you algorithm is so cool 👍 @MahmoudRizk

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes make it two separate operators.

Also, could you also describe how the algorithm works and why it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i will separate it.
About the Algorithm it is called "Double division algorithm", you can check this link http://www.doubledivision.org/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link. That is an awesome algorithm indeed.


if(sub_p<look_up[i] && i!=0){
tmpx1=look_up[i-1];
tmp_quotient = std::pow(2,i-1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 1<<(i-1) is an easier and faster choice. pow in cmath is for float number

@MahmoudRizk
Copy link
Contributor Author

I will look at it @titansnow

@karlphillip
Copy link

Anything new in this topic? Having a division operator would be great!

@kasparsklavins
Copy link
Owner

@karlphillip Before going over any PR's I want to add an automated test suite to reduce regressions. Havent had any time lately; not sure if this is used in production anywhere so I havent rushed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants