• Post Reply Bookmark Topic Watch Topic
  • New Topic

Code review - HugeInteger class  RSS feed

 
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
We had a president that said:

I'm never mistaken and I rarely have doubts.


Even though he, of course, is free to think like that, i don't agree with him at all! I believe that one must be humble enough to admit His/her limitations. I try to be like that!
If we see all the posts from Junilu so far in this thread, 99% of what is writen is about how to make code meaningfull, correct and easy to read! I don't honestly think this is a waste of time! It's always better to get the good habits early on in the learning process because we all know that the bad habits, when they settle, are very hard to correct!
So, yes, thank you all for all the advices given!
Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Knute Snortum wrote: The above method will allow you to set invalid signums.


Thanks Knute! I'll take a look at it when i have a bit of spare time!
 
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I disagree with the sentiment that return must always be the last statement in a function, referenced here.

Depends on the method in question, but simply advocating "never do it" is silly, IMO. There are benefits of multiple return statements.. like, less variables required, and less nested, highly indented code, and thus, greater readability.
 
Damon McNeill
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I also must disagree with many of the points of the style guide referenced here.

There's a reason for labelled break and continue statements. Maybe if the authors of this guide had their way, they'd re-write Java to create syntax errors out of the "bad" constructs. Then, it wouldn't be a language I would use again. There's a valid use case for break, continue, return, and even goto: that's why they exist in the first place.
 
Damon McNeill
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
However, I think other than the points I already mentioned, it's a pretty good guide. I especially agree with getting rid of the abomination of Hungarian notation. Nothing good ever came out of it! And many, many bad things like the Windows API. Yuck.
 
Damon McNeill
Ranch Hand
Posts: 67
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
For example, I got a lot of use out of using labeled continue and break statements, as well as the default fall-through behavior of a switch case. In my case, there isn't any way to perform the same operations as efficiently as not having them. So I was happy to rely on those tools.
 
Marshal
Posts: 56902
174
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Unfortunately labelled break and continue can produce code difficult to read or understand, and maintainability shou‍ld usually take precedence over execution speed. Please show an example of fall‑through because some people on this forum will be unfamiliar with it.
 
Sheriff
Posts: 4432
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The compareMagnitude() method has a problem: it doesn't correctly identify when the two magnitudes are equal.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi all!

Sorry if took so long to come back but i started new job on wednesday and hadn't have much time since.
I have made significant changes to my HugeInteger class. Tried to incorporate all that have been said by you guys and corrected some problems with the code.

The new version is here.

Haven't had time to implement the operations (add, subtract, multiply, divide and remainder). They need to be rewrited because the old versions won't work with the changes i made.
Honestly didn't test this version extensively so far... (again not much time).
Hope you'll find this version better than the oldest (which still can be found here).

Thanks for all your valuable input.

Best regards
Carlos
 
Knute Snortum
Sheriff
Posts: 4432
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I stumbled on this in your code:...not good!  Always use the equals() method to compare Strings. But also, Strings aren't a good way to show validity.  Use a boolean, and populate an error message if necessary.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Knute: Thanks. I'll take a look at it.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Knute: The way i thought that was to return the error message from the validation. You told me to use a boolean instead and populate an erros message if needed.
The boolean part is ok, i understand. But how to populate an error message when they can be different and that verification is done in the called method and not where the the exception is actually thrown...
For now i can imagine 2 ways of doing this:
1 - use a boolean return value from the called method and store the exception message in a String class field;
2 - use an int return value from the called method that indicates the index of the exception message stored in a String[] class field. Here the validity could be checked with a negative return value for a valid verification and a return value >= 0 for the message itself...

Are these good ideas?

Btw, if it's not allowed to put links for code files, how can i share it since it's a lot to post with code tags here?

Thanks

Best regards

Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sorry... I ment "by the way" and not "btw".
 
Knute Snortum
Sheriff
Posts: 4432
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

1 - use a boolean return value from the called method and store the exception message in a String class field;
2 - use an int return value from the called method that indicates the index of the exception message stored in a String[] class field. Here the validity could be checked with a negative return value for a valid verification and a return value >= 0 for the message itself... 


I think number (1) would be plausible, but I know many programmers here feel that class fields are code smell.  No, it's no good.  You would have only one error message per class, instead of per instance.  You need an instance field.

I don't like (2); I think it's because you would need magic numbers for the indecise. 

One method I'm thinking of is to have a static inner class that has a boolean and a String field, and return an instance of this class.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Knute Snortum wrote:One method I'm thinking of is to have a static inner class that has a boolean and a String field, and return an instance of this class.


Ok... I remember that i read somewhere already about inner classes. But don't remember where nor how to implement them! Have to search for it and learn them. Thanks Knute
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good morning all.
Made some more changes in my code. As Campbell said, i'll try to post it here with code tags. Sorry if it's lengthy...

 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The return description in the JavaDoc comments for the validateArray() and validateString() methods isn't right. Should be like this:
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Just implemented the methods for the multiplication:

Just noticed that the return statements of the getAddition() and getSubtraction() methods were wrong. Should be like this:
in getAddition():
In getSubtraction():
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sorry guys... Had a problem in the multiplication method... 
 
Knute Snortum
Sheriff
Posts: 4432
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here's a JUnit test for HugeIntegers that I have been building on over the days.  It currently show seven failures.

If you can run JUnit tests, then this will give you a great opportunity to test your class.  If you can't run JUnit tests, I can wlak you through it.

 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Knute: Thanks for the reply. Honestly i don't know how to run this kind of tests. Here where i'm coding i can't install applications so i'm using the portable version of BlueJ.
If i copy this code to the main method of a test class will it work? I'll try...

Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ok. I copied your code to a new class and it recognized it as JUnit tests.
I'm going thru the errors. But i found out allready an error in your test for opposite() method. You have: and sould be: Thanks
 
Sheriff
Posts: 11603
187
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
A cow for the unit test suite and a cow for finding and reporting a bug in it. 
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I had an error in my isLessThan() method. Was missing an else in line 18 below:Next... I made a significant change from my first version of the HugeInteger class: now a ZERO HugeInteger is defined with an empty array (length = 0). That's why this test below is failing:It's made for the first version where empty arrays weren't allowed. Now it won't throw a NumberFormatException but create Zero HugeInteger instead.
Finally (for now) this test:I think its not right. You are subtracting the minHuge from the maxHuge. But, by the properties of the signs in a subtraction, in fact you're adding them instead:
And that's why my code is throwing an ArithmeticException (Overflow). The result of this addition exceeds the capacity of the HugeInteger.

There is still 2 errors in my code... But i can't look at them now because i'm going to a meeting. Tomorrow i'll take a look...

Thanks Knute for the JUnit tests. They are indeed great!

Best regards
Carlos
 
Knute Snortum
Sheriff
Posts: 4432
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That's for going through my JUnit test and finding bugs in it.  I'm glad it's helping.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Junilu: Thanks for the cow!

@Knute: They are helping yes! A lot! Thanks again.
 
Knute Snortum
Sheriff
Posts: 4432
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I took all your suggestions into account and now I have two errors and one failure.

This test fails: And these lines produce array out of bounds exceptions:
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yeah... i know there are still errors... I'll look at them tomorrow.

I started to look at the error of the bad string but didn't have time to find the bug...

Thanks
 
Knute Snortum
Sheriff
Posts: 4432
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Something that would be good to add to your HugeInteger class is overridden equals() and hashCode() methods.  Overriding these methods in Object is important when sorting or creating Maps and you have to override hashCode() when you override equals().
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Knute Snortum wrote:Something that would be good to add to your HugeInteger class is overridden equals() and hashCode() methods.  Overriding these methods in Object is important when sorting or creating Maps and you have to override hashCode() when you override equals().

Hum... Overriding equals() means that my isEqualTo() will no longer be needed right? I'll look at it. I think that the most difficult will be the hashCode() one... I don't have a clue on how to do that... Let's see if i can come up with something.
 
Campbell Ritchie
Marshal
Posts: 56902
174
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good Evening
Why do your int[] and int fields not have private access?
Why are you creating a nested class for validation? If you have an invalid argument, why not throw an Exception? In line 135 you appear to use that nested class, but the validation method appears not to be in the validation object.
I don't think you shou‍ld write // comments about nested classes, or constructors. Those things shou‍ld be obvious to your readers anyway.
Line 82 is incorrect because you don't have a default constructor. But why do you want a no‑arguments constructor in the first place?
In line 165, what will happen if I pass a String argument line, "1234567890abcdef"? What do you expect from the numeric value method?
Why have you overloaded the constructor to accept an array in little‑endian format in one and big‑endian in the other? Why have you matched those constructors with factory methods taking the same arguments?
As somebody else has suggested, don't create an equals method which takes huge integer as a parameter. Also you will have problems with that method because it cannot cope with null as an argument.
Rather than isLessThan and isGreaterThan methods, I think you shoul‍d implement the Comparable<HugeInteger> interface.

Not enough time to say anything else now.
 
Sheriff
Posts: 22967
43
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos Reves wrote:Overriding equals() means that my isEqualTo() will no longer be needed right? I'll look at it. I think that the most difficult will be the hashCode() one... I don't have a clue on how to do that... Let's see if i can come up with something.



I expect your equals() method is going to be using some subset of the attributes of a HugeInteger as keys for comparison, right? Then I would suggest using the Objects.hash() method to compute the hash code; pass it a list containing the same attributes you used for equals(). Remember that equals() and hashCode() should be defined to be consistent with each other. (My paraphrase; read the docs for Object.hashCode() for the real requirement.)
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good morning all!
@Campbell: A lot of questions...  I'll try to answer them.

But first just one thing. As i said in the beginning of this thread this is an exercise for me to learn new things. It's not ment to be used for nothing more than that. So yes, many things might be wrong or not done "in a professional way" so to speak. I try to incorporate the suggestions made by all here and, when new things are thrown i try to learn how to implement them. That being said, let's see your questions.

Campbell Ritchie wrote:Why do your int[] and int fields not have private access?

Very good question! They should indeed be. I had them private but not final. Then i thought they should be final and made that change but it didn't compile because i had them initialized outside the constructors. So i changed that also. But for some reason i overwrited the private with final. I'll correct that.

Campbell Ritchie wrote:Why are you creating a nested class for validation? If you have an invalid argument, why not throw an Exception? In line 135 you appear to use that nested class, but the validation method appears not to be in the validation object.

It's not quite for validation but to pass the results of the validation. I didn't want to put the validation inside the constructors so that they wouldn't be lengthy. So i created methods for that. Then i had several Exceptions being thrown in those methods when an invalid argument was passed and i thought why not have just one place to throw them since they all were NumberFormatExceptions and only their message was different depending on what the problem was. So i put throw statement in the constructor but then had a problem: had to pass the result of the validation and the message to be passed to the thrown Exception. After talking about that here with Knute, his thoughts were that an instance variable would do the trick but he suggested that a static inner class could be an answer too. So, since it was a new thing for me, i searched for it and tried to implement it.

Campbell Ritchie wrote:I don't think you should write // comments about nested classes, or constructors. Those things should be obvious to your readers anyway.

You mean those // comments i made to say where the different types of things start (nested classes, constructors, public methods and so on)? Because beside of those and the JavaDoc comments there are no other in the nested class or the constructors.

Campbell Ritchie wrote:Line 82 is incorrect because you don't have a default constructor. But why do you want a no arguments constructor in the first place?

You're rigth. Bad choice of words. The default constructor is the empty one that's inherited from the Object class when we don't provide our own constructors right? But i thought of this argumentless constructor as a way to create a zero HugeInteger.

Campbell Ritchie wrote:In line 165, what will happen if I pass a String argument line, "1234567890abcdef"? What do you expect from the numeric value method?

There's a problem with that method... That's the failed test from Knutes suite... I'll have to look at it.

Campbell Ritchie wrote:Why have you overloaded the constructor to accept an array in little-endian format in one and big-endian in the other? Why have you matched those constructors with factory methods taking the same arguments?

The one in big-endian format is public. I think it's much more intuitive for someone that creates a HugeInteger to pass the number 1234 as {1, 2, 3, 4} than as {4, 3, 2, 1}. The little endian one is private and only used internally. I thought of that because if my digits array is already in little-endian format and it's supposed to be allready verified and correct, why the need for convertion to big-endian, the aditional validation and, again, the convertion to little-endian?

Campbell Ritchie wrote:As somebody else has suggested, don't create an equals method which takes huge integer as a parameter. Also you will have problems with that method because it cannot cope with null as an argument.

Ok. I'll take that in consideration. But as i said, that and the hashCode() are new thing for me to research...

Campbell Ritchie wrote:Rather than isLessThan and isGreaterThan methods, I think you should implement the Comparable<HugeInteger> interface.

And yet another new thing for research!
Thanks Campbell for your inputs!

Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote:I expect your equals() method is going to be using some subset of the attributes of a HugeInteger as keys for comparison, right? Then I would suggest using the Objects.hash() method to compute the hash code; pass it a list containing the same attributes you used for equals(). Remember that equals() and hashCode() should be defined to be consistent with each other. (My paraphrase; read the docs for Object.hashCode() for the real requirement.)

Thanks Paul! I'll consider your advice also!

Best regards
Carlos
 
Campbell Ritchie
Marshal
Posts: 56902
174
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos Reves wrote:. . . It's not quite for validation but to pass the results of the validation. I didn't want to put the validation inside the constructors so that they wouldn't be lengthy. So i created methods for that. Then i had several Exceptions being thrown in those methods when an invalid argument was passed and i thought why not have just one place to throw them since they all were NumberFormatExceptions and only their message was different depending on what the problem was. So i put throw statement in the constructor but then had a problem: had to pass the result of the validation and the message to be passed to the thrown Exception. After talking about that here with Knute, his thoughts were that an instance variable would do the trick but he suggested that a static inner class could be an answer too. So, since it was a new thing for me, i searched for it and tried to implement it.

That does look as though you are letting the error messages guide your programming. Don't. Crack the whip over the error messages and make sure they are scared of you, not you of them.
It is good to learn about nested classes, but I don't think one is necessary here. I would suggest you have one validation method, with private access, called from every constructor taking a String parameter.Note I used %s not %d, because the input String is not in the format for a number, and you don't want to get a different Exception thrown. That Exception will carry all the information you want in its error message. You can of course add more details to the error message. You can have a second parameter for the validation method and pass part of a message to describe which constructor it was called from.

This goes to show you that there are always multiple opinions about the best way to do things.

Do you really want a no‑arguments constructor? I like constructors, but I firmly believe that fewer constructors means less opportunity for things to go wrong. A constructor creating a default value of 0 is a valid thing to have.
I am a bit worried that changing between big‑endian and little‑endian is simply making things more difficult for yourself, but there is probably n point in changing it now.

  . . . Thanks Campbell for your inputs!

Best regards
Carlos

That's a pleasure Well done taking my comments in good spirit.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:That does look as though you are letting the error messages guide your programming. Don't. Crack the whip over the error messages and make sure they are scared of you, not you of them.

  (if there was a whip emoticon i would have put it here... ). it may seem that but it wasn't the message itself that made me change. It were the multiple throw statements. I wanted a way to have only one, not a bunch of them.

Campbell Ritchie wrote:It is good to learn about nested classes, but I don't think one is necessary here. I would suggest you have one validation method, with private access, called from every constructor taking a String parameter.Note I used %s not %d, because the input String is not in the format for a number, and you don't want to get a different Exception thrown. That Exception will carry all the information you want in its error message. You can of course add more details to the error message. You can have a second parameter for the validation method and pass part of a message to describe which constructor it was called from.

This goes to show you that there are always multiple opinions about the best way to do things.

Yeah. I already noticed that there are a lot of gifferent approaches for the same thing. But hey, this is a good way to know them, try them and then see which is the one i like most. As i said, it was good to learn about nested and inner classes and see them in action with my own implementation. To have all constructors calling the same validation method with a String argument, i will have to transform the int array in a String though...

Campbell Ritchie wrote:Do you really want a no arguments constructor? I like constructors, but I firmly believe that fewer constructors means less opportunity for things to go wrong. A constructor creating a default value of 0 is a valid thing to have.

Sorry if i didn't quite understand your point here... It seems a bit contradictory to me, or perhaps it's my english playing tricks on me... You say that a constructor creating a default value of 0 is a valid thing to have but you ask if i really want a argumentless constructor... It was the most intuitive way i found to do it, that's all...

Campbell Ritchie wrote:I am a bit worried that changing between big-endian and little-endian is simply making things more difficult for yourself, but there is probably n point in changing it now.

Well having the array in little-endian (LE) format does make the mathematical algorithms a bit less difficult to implement(or at least less confusing), especially the multiplication one... In fact in my first version of the code i converted to little-endian in the multiply() method prior to the multiplication itself, and then converted back to big-endian. I just thought that having it implemented in LE from the start would make the code more coherent.

Campbell Ritchie wrote:Well done taking my comments in good spirit.

I'm here to learn in the first place. All opinions/advices/comments are welcome! You guys make them, and i try to explain what was behind my approach!

Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:I like constructors, (...)

Just one more thing about the different oppinions on how to do things...
I was reading about the equals() and hashCode() implementation and found this book Effective Java (2nd Edition) by Joshua Bloch. Although this is a book that is a bit over my head for now (i'm being kind with myself here...), it's interesting to see that he prefers static factory methods instead of constructors.
 
Campbell Ritchie
Marshal
Posts: 56902
174
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos Reves wrote:. . . if there was a whip emoticon i would have put it here. . . .

Hahahahahahahahahahahaha! As long as you don't hit me with the whip emoticon.

. . . To have all constructors calling the same validation method with a String argument, i will have to transform the int array in a String though...

No, you can consider a different validation method taking array parameters.

. . . I'm here to learn in the first place. All opinions/advices/comments are welcome! You guys make them, and i try to explain what was behind my approach!

Best regards
Carlos

You are doing well taking the comments
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!