• Post Reply Bookmark Topic Watch Topic
  • New Topic

Code review - HugeInteger class  RSS feed

 
Marshal
Posts: 58830
179
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos Reves wrote:. . . this book Effective Java (2nd Edition) by Joshua Bloch.

It might be difficult to read, but it is a really good book.

. . . he prefers static factory methods instead of constructors.

That may be a bit of an exaggeration. Bloch points out that factory methods have advantages, and constructors have disadvantages under some circumstances, but he doesn't actually say to prefer factory methods over constructors. At least I don't think so. In the present instance there is no need to abandon public constructors.

. . . Now, on the other hand, if you had factory methods taking int[]s in big‑endian and little‑endian form, that probably would justify making all your constructors private and making instances available only via factory methods. Remember that factory methods are usually static. Many Java8 classes use factory methods throughout, as you will find in this part of the Java™ Tutorials.
 
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good morning all!
So i made some changes in code to eliminate the failing tests from Knutes suite. Eliminated the nested class and the result was this:
Now it seems to work correctly.
Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Added these tests to Knutes suite to test the multiplication. They all ran successfully.

 
Sheriff
Posts: 4840
136
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Cows for a great job fixing your code and adding to the test suite.

A note about calling methods from constructors.  Any method you call from a constructor should be marked final.  This is because if the method gets overridden it could cause strange behaviors in your constructor.
 
Knute Snortum
Sheriff
Posts: 4840
136
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I don't have your new add() and subtract() methods, so I'm still getting errors in them, and I don't have multiply().  Do you want to keep posting here, or are you satisfied?  In a real work environment we would be posting to some sort of versioning software like git.  You can do this easily with github.com for free.
 
Knute Snortum
Sheriff
Posts: 4840
136
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
In the method validateString(), this line seem to do nothing:
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Knute Snortum wrote:Cows for a great job fixing your code and adding to the test suite.

Thanks Knute. Appreciate!

Knute Snortum wrote:A note about calling methods from constructors.  Any method you call from a constructor should be marked final.  This is because if the method gets overridden it could cause strange behaviors in your constructor.

Ok... Didn't know that. Will take a look.

Knute Snortum wrote:I don't have your new add() and subtract() methods, so I'm still getting errors in them, and I don't have multiply().  Do you want to keep posting here, or are you satisfied?  In a real work environment we would be posting to some sort of versioning software like git.  You can do this easily with github.com for free.

I have an account on GitHub. If i upload there my code i can post the links to it here? I'm almost done for now. But already made the divide() and remainder() methods also. And added tests for them in the suite (all went well).

Knute Snortum wrote:In the method validateString(), this line seem to do nothing:

You're right. I used it in an earlier version but now it's not used at all. Forgot to delete. Done...

Best regards
Carlos
 
Knute Snortum
Sheriff
Posts: 4840
136
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

I have an account on GitHub. If i upload there my code i can post the links to it here? 


You can.  I don't think any of the moderators have a problem with a link to GitHub, but some of them might want you to post the code directly.  Still, with all the back and forth I think posting it to GitHub makes sense.  Post your version of the testing program too.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good afternoon all.

I've uploaded my files to GitHub. Here is the link.

Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Don't know who gave me the cow in my previous post. But thanks!
 
Creator of Enthuware JWS+ V6
Bartender
Posts: 3094
255
Android Chrome Eclipse IDE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Congratulations    your question has made it to our CodeRanch Journal - February 2017

Have a Cow!
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Frits Walraven wrote:Congratulations    your question has made it to our CodeRanch Journal - February 2017

Have a Cow!

Wow! That's great! Thanks for the journal entry and for the cow!
Best regards
Carlos
 
Saloon Keeper
Posts: 8766
163
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I took a look at the code on GitHub. Here are some suggestions:

  • Always use a package statement. Using the default package almost always leads to problems at some point down the line.
  • Make your class final. There's probably no reason to extend the HugeInteger class. You then don't have to make your methods final.
  • Make your fields private. Right now digits is an int[], but it could become difficult to change its type if other classes start using fields directly.
  • I'm not sure why the internal representation is in little endian form while the class' client almost always works in big endian. There's a lot of data juggling going on without real benefit.
  • There's no point in having a nullary constructor. It's only used to instantiate ZERO, and that can easily be done using any of the other constructors.
  • Don't make your constructors public if you have factory methods.
  • You can avoid a lot of code duplication by letting more specific constructors/method call more generic constructors/methods.
  • All methods that don't operate on instance fields can be static.
  • I would rename parse(int[]) to ofDigits(int[]). Parsing generally occurs only on strings. I would then rename parse(String) to ofString(String) as well to make the class consistent in its use. Both of these methods could have another int argument that specifies the radix of the digits.
  • you should rename opposite() to negate().
  • You don't need isOne(), isZero() and isMinusOne() if you make your class instance-controlled.
  • Instead of a clone() method, make a copy-constructor. This avoids confusion with the Object.clone() method. It can be private because there's no reason to clone an immutable class.
  • Override equals() and hashCode().
  • Let your class implement Comparable<HugeInteger>. You can then implement all your relational operators in terms of the compareTo() method.
  • If your class is immutable, you don't have to create clones when working with the additive and multiplicative identities.
  •  
    Carlos Reves
    Ranch Hand
    Posts: 116
    10
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    @Stephan: Thanks for the inputs Stephan. I'll take a look.

    Best regards
    Carlos
     
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!