Win a copy of Cross-Platform Desktop Applications: Using Node, Electron, and NW.js this week in the JavaScript forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Code review - HugeInteger class  RSS feed

 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good morning all!

I've been reading Java How to Program: Early Objects, 10/E and at some point there is an exercise to create an HugeInteger class.
The exercise is as follows:
Create a class HugeInteger which uses a 40-element array of digits to store integers as large as 40 digits each. Provide methods parse, toString, add and subtract. Method parse should receive a String, extract each digit using method charAt and place the integer equivalent of each digit into the integer array. For comparing HugeInteger objects, provide the following methods: isEqualTo, isNotEqualTo, isGreaterThan, isLessThan, isGreaterThanOrEqualTo and isLessThanOrEqualTo. Each of these is a predicate method that returns true if the relationship holds between the two HugeInteger objects and returns false if the relationship does not hold. Provide a predicate method isZero. If you feel ambitious, also provide methods multiply, divide and remainder.

I've been trying to code it.
Although it's not explicit in the exercise not to use the BigInteger class to implement this one, i resisted the temptation of using it, and tried to implement all the methods with my own algorithms.
Apart from all the methods asked in the exercise (from which i didn't code yet the divide and remainder because they are a bit harder), i've implemented some other public methods: toString, isOne, isMinusOne, isPositive, isNegative, abs, opposite and copy.
All my code compiles and works fine (but of course is not the best in terms of performance...).
I wanted to ask if someone could take a look at it and give me oppinions/advices on it. I don't put code in here because, as you can imagine, it's lengthy and it doesn't make sense to put just a part of it.
But i can upload the .java source file if someone is willing to take a look!

Thanks in advance
Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The source code can be obtained here.

Best regards
Carlos
 
Knute Snortum
Sheriff
Posts: 3940
92
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nice formatting and comments.  Generally looks good.

* Don't import java.lang.*, that's automatically done.  import java.util.* is unused.  In general, I don't like wildcards in import statements.  Be explicit.

* I'd rather see you throw an InvalidArgumentException is the arguments are bad.

* This code confused me:
Why the test for whether the character is a digit?  And what about if the string starts with a "+"?

* 'this' is generally used only to disambiguate an argument from a field.  You don't need this.someMethod().

* 'this' is just plain wrong when calling static methods.

* Your toString() method should use StringBuilder

* I think you have a bug in this method:
What would this method return if it were passed { 1, 2, 5, 1 } and { 1, 2, 4, 9 }?

* Is there an elegant way that add() and sub() can call a common method?  There's a lot of common code.

* What happens in intAdd() if two maximum numbers are passed? (10 ^ 40 - 1)

* Same with intSubtract(): What is two minimum numbers are passed?

* Can intAdd() and intSubtract() call a common method?

Looks like you started multiply and divide but didn't finish.

I may have more comments when I test the class.
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If you can spare the reader some effort by choosing better names, do it. You may think that it's clear that "mag" is short for magnitude but it's not immediately apparent to others. Besides, making the reader subconsciously complete the word every time they read the abbreviation is inconsiderate at best. Understanding code is hard enough without having to spend extra brain power trying to fill in the blanks. Do others a favor and give them full words to read.
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There are, of course, notable exceptions to what I just said about abbreviations. That, I will concede. For example, rand is a very commonly used name for an instance of the Random class. The name kbd is another such name, although not as common and less preferable to keyboard and input. The name mag is kind of in the same boat as kbd except, for me at least, it's clearly less preferable than magnitude.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Knute: thanks for the reply and comments. Will try to answer them.
Knute Snortum wrote:* Don't import java.lang.*, that's automatically done.  import java.util.* is unused.  In general, I don't like wildcards in import statements.  Be explicit. 

Will remove both. At a given time i used the Arrays class but then i changed my mind and forgot to remove the java.util.* import. And i'll try to be explicit in imports in the future.
Knute Snortum wrote:* I'd rather see you throw an InvalidArgumentException is the arguments are bad.

I wanted to initialize to zero if the arguments are invalid. If i throw an Exception at this phase it won't do that becaus, as you can see, i'm not handling exceptions for now. Just throwing them...
Knute Snortum wrote:* This code confused me:
Why the test for whether the character is a digit?  And what about if the string starts with a "+"?

Well since i verify the string for correctness early on in that method i know that it could start only with a digit, a minus sign or a plus sign. This check is to see if it's one of those signs. if it's a minus i have to remember it to set the correct signum later. If it's a plus i don't need to "remember" it since i assume that if isn't minus must be plus (of course it can be zero also but the next check does that). To check if it isn't a digit also is needed to remove the sign out of the string with the line:
Knute Snortum wrote:* 'this' is generally used only to disambiguate an argument from a field.  You don't need this.someMethod().

* 'this' is just plain wrong when calling static methods.

Ok... Will correct that.
Knute Snortum wrote:* Your toString() method should use StringBuilder

I really thought of that also since there may be many concatenations to be made... But started to code the other things and just never remembered that again...
Knute Snortum wrote:* I think you have a bug in this method:
What would this method return if it were passed { 1, 2, 5, 1 } and { 1, 2, 4, 9 }?

Yes... It gives the first less than the second... which is clearly wrong... Have to look at it...
Knute Snortum wrote:* Is there an elegant way that add() and sub() can call a common method?  There's a lot of common code.
* Can intAdd() and intSubtract() call a common method?

Certainly can for those common tasks they perform. Will take a look at it.
Knute Snortum wrote:* What happens in intAdd() if two maximum numbers are passed? (10 ^ 40 - 1)

* Same with intSubtract(): What is two minimum numbers are passed?

Will take a look. To be honest didn't test that case...
Knute Snortum wrote:Looks like you started multiply and divide but didn't finish.

Yes... As i mentioned in my first post i didn't code the division yet so neither divide nor multiply fully work. I'm struggling with the division algorithm... It's not as easy as the multiplication one...

Thanks for all the insights. I'll try to get it better!
Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Junilu: Thanks for the tip! I'll chage that mag to magnitude and see if there are other like that one...

Best regards
Carlos
 
Piet Souris
Rancher
Posts: 1943
66
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
What is the advantage of using an array instead of an ArrayList? And why limit te magnitude to a max of 40?
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Piet Souris wrote:What is the advantage of using an array instead of an ArrayList? And why limit te magnitude to a max of 40?

Well, the exercise says that. To use an array of 40 elements.

 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I've corrected the compare method:


I think now it's working as it should.
The problem was that initially i had return statements inside the for loop. They made the code exit the loop at the first integer that was less or greater in the first array.
But i've been reading the page Code Formating and there is said to avoid it and have only one return statement at the end of the method.
So when i changed it accordingly it wouldn't already exit right away and proceed with the rest of the loop. But i think that now it works. Now just a bit of fine tunning is needed i think...

Bets regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'll stay focused on names for now: 

I get why you'd do this and use magnitude for the name of the array of int. However, you also have to think of the semantics of magnitude[i]. Do you read that and naturally understand that to mean the I-th "chunk" of the huge integer? I'm just using "chunk" here now for lack of any better word but if I understand the concept correctly, you're going to use a set of regular int values to present chunks of a bigger number. Since ints are 32-bit values and let's say I had two ints in this array, with the bit positions numbered from 0 starting at the least significant bit (the bit on the right end of the binary representation of the value), the first int might represent the higher-order bits 63-32 and the second bit might represent the lower-order bits 31-0. The HugeInteger would be the value represented by the combination of all these chunks of bits and whatever the sign is.

If I'm correct on this, then I'd probably prefer to name this array something like segments instead.

By the way, the representation I describe above with the 0-th element of segments representing the highest order bits of the value and each succeeding element representing progressively lower-order bits of the value is called "Big-Endian" ordering. That means if you iterate normally over the array from the first to the last element, you will start with the big end of the value and work your way through the array towards the little end of the value. The opposite ordering is called Little-Endian. The choice of big- vs. little-endian ordering will affect how you process the segments array when performing calculations like addition and multiplication, particularly in how you carry over bits from one segment to another.

As for the signum field, I'd probably prefer to name it sign instead. I'd question the choice to make it represent 0 because you can determine that from the segments array.  If the sign field also had to represent 0, then you'd be violating the DRY principle (Don't Repeat Yourself) because you'd have two representations of the same idea in your code. You'd either have to write code that gave priority to the sign or the segment field to determine whether the value represented by the HugeInteger is zero or you'd have to keep the two fields in synch, that is if you assign 0 to sign then you'd have to also zero out the int array segments so that they're consistent with each other.  I think that's way too troublesome and error-prone. Violations of the DRY are open invitations for bugs to infest your program.

If feel you need to document these fields with comments, I'd document them as a pair of co-dependent fields.

Also, I'd probably add some private methods to help manage the value of sign:
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Method toString() with StringBuilder:


@Junilu: I'm going to a metting now but when i have a bit of time i'll look at your remarks... Thanks!

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sticking with making code more meaningful, let's look at this:

The name val is generic and it's an abbreviation. As far as being an abbreviation, not so smelly but still in the same league as kbd and mag. Focus first on its expressiveness though. This is the divide() method. You can name the parameter divisor instead and this will make it crystal clear what the value represents in terms of the division operation.

As for line 786, throwing a NumberFormatException doesn't make much sense.  Division by zero is reported in other standard Number classes as an ArithmeticException with a message of "/ by zero" (try it with the Integer wrapper class). Try to stay consistent with the behavior of other classes in similar situations. Don't make your objects violate the Principle of Least Astonishment/Surprise by being a non-conformist and doing its "own thing" -- that may be cool if you're trying to impress the ladies but it's not cool when you're designing an object.
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Again, I'd take some cues from the standard library wrapper classes. The java.lang.Integer class defines two similar static methods (note the difference in the declared return type, however):

In your case, defining an instance method for parsing a string into a HugeInteger implies one thing: HugeInteger is not immutable (Edited - see my next reply).  I don't know if you were supposed to design it as such but you may want to consider what it means to have a class that's immutable vs mutable. The standard library's numeric wrapper classes are immutable.

EDIT: Also, it's better to put JavaDoc style comments before the method. The comments you have there talk about implementation details though so you'd either want to get rid of those and make your code clearer so it actually clearly expresses the same thing or you'd add an implementation note. You can do one of the alternatives I show below but it's probably not a good idea to do both:
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I've seen more code where you use that internal intParse() method so it looks like I have to take back my previous statement that the intParse() method makes your HugeInteger class mutable. If it's called only from the constructors, then I was wrong.

However, this code could be improved if you took some cues from the java.lang.Integer and the java.math.BigInteger JavaDoc comments.

Better:
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good morning all!

@Junilu: Wow... a lot to process... Thanks for all these sugestions. But sorry if only now i'm answering. Yesterday was my birthday and after the meeting i went home to be with family...
But starting with the first post you made and concerning the magnitude and signum fields. I indeed did resist the temptation of using BigIntegers to implement this HugeInteger class. But that doesn't mean that i wasn't curious to see how BigInteger is actually implemented! So i went to this site at grepcode. There i saw those two designations and i got curious about them. So i searched about them and found out about magnitude and about the Signum Function (or Sign Function). That was why i decided to use them also. I used -1, 0 and 1 for the signum because this is how this function is defined as you can see in the page.
I don't mean that i can't use better designations for them, only want to explain how i got there.
I agree that it's more work to mantain the sign = 0 consistent with the magnitude = {0}, and i tried that trhough out the code, in fact when i wanted to see if the hugeinteger was 0 i always used the getSignum method and never touched the magnitude array.

Regarding Big-Endian (BE) vs Little-Endian (LE) my magnitude is BE indeed. But when i started to code the intMultiply method i realized that it would be much easier if it was in LE, so that's why i convert it to LE there before multiplying. I could implement my magnitude array as LE but if i do that i'll have to do massive changes in my code.

I'll think about these questions and see what i'll come up with.

Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu wrote:The name val is generic and it's an abbreviation. (...)

When you spoke about the mag name, i corrected that in my code and started to look at all the other abbreviations i had in my code. Val was one of them that i use a lot. I'm doing those changes all over the code to make the names more meaningfull.

Junilu wrote:As for line 786, throwing a NumberFormatException doesn't make much sense.

Yes, i totally agree with you. To be honest i didn't give much thought yet on those exceptions. I just threw them where i thought they would fit and they are all the same NumberFormatException. I'll change it to an Arithmetic Exception.

Junilu wrote:that may be cool if you're trying to impress the ladies but it's not cool when you're designing an object.

My wife wouldn't like at all that first part!
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu wrote:I've seen more code where you use that internal intParse() method so it looks like I have to take back my previous statement that the intParse() method makes your HugeInteger class mutable. If it's called only from the constructors, then I was wrong.

Didn't quite understand what you said here... So i have a muttable or imuttable class? (my english sometimes tricks me...)
I know that all standard library's numeric wrapper classes are immutable (although the BigInteger class uses an internal muttable class, that is used only inside the package, to perform some operations).
I think that maybe i failed to acomplish that... the magnitude and signum fields would have to be final for that right?

I'll take a look at all your suggestions and do some changes accordingly. I don't know if i'll be abble to make them today though, because i have a slight busy day here at work, but when i make them i'll upload an updated version of the code.

Thanks a lot!

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:I indeed did resist the temptation of using BigIntegers to implement this HugeInteger class. But that doesn't mean that i wasn't curious to see how BigInteger is actually implemented! So i went to this site at grepcode. There i saw those two designations and i got curious about them. So i searched about them and found out about magnitude and about the Signum Function (or Sign Function). That was why i decided to use them also. I used -1, 0 and 1 for the signum because this is how this function is defined as you can see in the page.

Huh, you learn something new every day.

Now, what I'm about to say here may sound a bit like backpedalling or just being obstinate to try to save face. However, I'm old enough now to not care too much if I do something that other folks might feel embarrassed about. I basically said, "Don't do this and that because of this and that." and you basically came back and said, "Well, I did that because Joshua Bloch and his guys did it over here..."  Naturally, I might be expected to say, "Well, I sure have some egg on my face now, don't I?" and in a way, I do. But I learned something new about BigInteger and about the math of big numbers and their binary representation so that's something to be happy about, not embarrassed. I'll even give you a cow for that little Valentine's gift.

My comments were motivated by my constant quest for clarity in code, which is always a good thing to pursue. Most code spends less of its lifetime being written than being read; in other words, code is read more often than it is written. So, you should try to focus more on making code readable than easier to type out by saving a few keystrokes on variable names. Choosing names that make sense and reveal their intent is also a primary pursuit of mine and I stand by its importance as a code-writing goal. (Here's the part that might sound like backpedalling) However, I guess a class like BigInteger spends most of its lifetime not being used (by me at least) and a lot less of its lifetime being read (again, at least by me, obviously). I can only imagine that the conversation to use the name mag went something along the lines of, "Let's just call this 'mag' so we don't have to keep typing out 'magnitude' all the time. Besides, not a whole lot of people are going to read this code anyway once we publish this." Since I'm being honest, I have to admit that if by some strange twist of fate I had a chance to help make that decision, I probably would have been persuaded to go along based on that reasoning. My lame excuse is that I'm not a mathematician. 
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:When you spoke about the mag name, i corrected that in my code and started to look at all the other abbreviations i had in my code. Val was one of them that i use a lot. I'm doing those changes all over the code to make the names more meaningfull.

Coding often involves negotiation and compromise. When you're coding by yourself, you may or may not take the time to weigh options and then make a judgement call that's based on the kind of values you have as a programmer. If you value brevity, then you might go with the name val instead. If you're like me, you'll easily convince yourself to use value. You have to keep this in mind as you develop your own style. My comments are made with a heavy bias for my style.

When I program with other people who have strong opinions of their own, the conversation and negotiations that come along with them can get a little more involved, sometimes even contentious. Let's imagine someone saying, "I know you don't like abbreviations but val is a pretty common one plus it's consistent with the name used in the standard library over here and there and a bunch of other places. I think it's Ok to go with 'val'." I probably would not choose to fight this battle but reserve my energy for when I need to argue against something that's more grating to my senses.
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I guess this thread is a good example of why we caution people about the use of the word "should" when they post replies—sometimes the choice is not black or white; it depends on the situation.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Junilu: First i don't think at all that you should feel embarrassed! As i said i was only explaining the reasons that were behind my decision on sticking with those names. Since it's a mathematical related class and those are mathematical concepts it made sense in my head to use them (not because Joshua Bloch and his guys did it over there. In fact i used magnitude for my field name and not mag as they did. I only used mag in the arguments of constructors and methods).
Don't get me wrong. i really understand the good logic behind your pursue for clarity in code and, honestly, i agree with it since as my coding experience is very little, i find it very hard to understand a lot of things that are made in BigInteger implementation...
I'm glad that you too learned a bit more! As far as i'm concerned, i'm learning a lot from you!

And thanks for the cow! really appreciate it!

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:I just threw them where i thought they would fit and they are all the same NumberFormatException. I'll change it to an Arithmetic Exception.

Yeah, the BigInteger JavaDocs even mentions throwing ArithmeticException when trying to divide by zero.

I don't know if I mentioned this yet but one other consideration that you might want to throw in there with clarity and expressiveness is "consistency". That has to do with the Principle of Least Astonishment/Surprise.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu wrote:f I'm correct on this, then I'd probably prefer to name this array something like segments instead.

Or maybe digits? Afterall each of magnitude[i] is a digit from the HugeInteger...
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:Since it's a mathematical related class and those are mathematical concepts it made sense in my head to use them (not because Joshua Bloch and his guys did it over there. In fact i used magnitude for my field name and not mag as they did. I only used mag in the arguments of constructors and methods).

Of course. Poor choice of words on my part; it's a reflection of what I was thinking, not in any way trying to take away from your ability to make your own decisions.

This reminds me of my Aikido class where I'll often learn something new about a technique from what someone who is less experienced does when they're partnered up with me. As you get more experience, you start doing things more consistently. Consistency is a good thing but it can also be the enemy, or at least a hindrance, to innovation and growth. White belts don't move the way more experienced students move and they often do things that experienced students don't expect. You can learn a lot of new things from the little wrinkles a beginner's naiveté can introduce into the dynamics of a technique. (Not that I'm calling you a naive beginner... I hope you get what I'm trying to say here)
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:
Junilu wrote:f I'm correct on this, then I'd probably prefer to name this array something like segments instead.

Or maybe digits? Afterall each of magnitude[i] is a digit from the HugeInteger...

I don't think digits fits what the array is about. Now that I know you're using BigInteger as a point of reference, "magnitude" is probably good enough, too.

Also, I'm not sure it's obvious (or even correct) to see magnitude[i] as representing one digit in a HugeInteger. The ints are converted to their binary equivalents and represent a segment of the HugeInteger's binary representation. That's why I thought of "segments" as an appropriate name.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Of course. Poor choice of words on my part; it's a reflection of what I was thinking, not in any way trying to take away from your ability to make your own decisions.

And i understood that. I never ment to say you were trying such a thing!
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:
I don't think digits fits what the array is about.
Also, I'm not sure it's obvious (or even correct) to see magnitude[i] as representing one digit in a HugeInteger. The ints are converted to their binary equivalents and represent a segment of the HugeInteger's binary representation. That's why I thought of "segments" as an appropriate name.

Are they being converted? In BigInteger i know they are (it's a byte array). But i use an int array...
That's why i thought digits would fit...

Junilu Lacar wrote:Now that I know you're using BigInteger as a point of reference, "magnitude" is probably good enough, too.

Well i did go there and see it... Honestly didn't understand much of the binary operations made there... So didn't even try it... for now! After that i didn't use it for nothing more since all i made was around integer manipulations...
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
(Just saw... I'm not a Greenhorn anymore... )
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:
Are they being converted? In BigInteger i know they are (it's a byte array). But i use an int array...
That's why i thought digits would fit...

I just realized why you'd think that. We weren't on the same page. I was still thinking in terms of binary representations. I see what you're getting at now. Another one of those wrinkles that I wasn't expecting. If that's how you want to use them, I suppose that's fine. I hesitate to say this because it's about optimization but to use an int to represent a value from 0-9 seems like a bit of a waste.  Your intent might be more obvious if you declared a byte[] instead. This is where I think an explanatory comment would be appropriate to explain why you're declaring this field with whatever type and name you decide to go with.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:I just realized why you'd think that. We weren't on the same page. I was still thinking in terms of binary representations. I see what you're getting at now. Another one of those wrinkles that I wasn't expecting. If that's how you want to use them, I suppose that's fine. I hesitate to say this because it's about optimization but to use an int to represent a value from 0-9 seems like a bit of a waste.  Your intent might be more obvious if you declared a byte[] instead. This is where I think an explanatory comment would be appropriate to explain why you're declaring this field with whatever type and name you decide to go with.

Yeah... I thought you might been thinking of binary representations... I could make that explanatory comment you suggest but... i believe it wouldn't be much of a flattering one...

See what i mean?
 
Liutauras Vilda
Marshal
Posts: 4483
304
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One small point:
When you read this line out loud, what you get is: "if not is less", now, consider having it just as "less", so you could read it "if not less".
It seems small thing, but in my opinion it improves code readability significantly, as makes you feel confident about the code you read, so you can be sure it indeed does what the spelt out words actually meant.

And have a cow for incredibly good code formatting. I like it a lot.
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:I could make that explanatory comment you suggest but... i believe it wouldn't be much of a flattering one...

See what i mean?

You don't have to be that self-deprecating. I actually have put comments like "Developer Note: just doing this for expediency right now; we'll need to refactor this once we know more about (the more complicated approach)" in my code just to remind myself and others what I was thinking at the moment.

By the way, if you're going to use that array to represent single digits, then it does make sense to name it digits. And I guess signum as used in BigInteger does stand for something that's not exactly reflected by the name sign very well, so if you want to go back to using signum that's probably fine, too, as long as you're going to use it the same way as it's used in BigInteger.  Remember, I didn't know what the significance of the signum name was when I commented while under the impression that it was supposed to just represent the sign.  Now that I know otherwise, I have no objection to signum.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:You don't have to be that self-deprecating. I actually have put comments like "Developer Note: just doing this for expediency right now; we'll need to refactor this once we know more about (the more complicated approach)" in my code just to remind myself and others what I was thinking at the moment.

Well i was joking a bit. But i don't see it as self-deprecating cause it's the honest truth! of course being the honest truth doesn't mean i would write it that way...
Junilu Lacar wrote:By the way, if you're going to use that array to represent single digits, then it does make sense to name it digits. And I guess signum as used in BigInteger does stand for something that's not exactly reflected by the name sign very well, so if you want to go back to using signum that's probably fine, too, as long as you're going to use it the same way as it's used in BigInteger.  Remember, I didn't know what the significance of the signum name was when I commented while under the impression that it was supposed to just represent the sign.  Now that I know otherwise, I have no objection to signum.

For now i won't work with the binary representations of the numbers. I'm trying to learn the basic. When my knowledge rises perhaps i'll try to enter that field (now it's still a very clowdy one...).

Liutauras Vilda wrote:One small point:
When you read this line out loud, what you get is: "if not is less", now, consider having it just as "less", so you could read it "if not less".
It seems small thing, but in my opinion it improves code readability significantly, as makes you feel confident about the code you read, so you can be sure it indeed does what the spelt out words actually meant.

I used isLess because the logic for that variable was "the first is less than the second" as you can see in the following lines (i presume you took this from my compareMagnitude() method). But i'll take a look.
Liutauras Vilda wrote:And have a cow for incredibly good code formatting. I like it a lot.

Thanks a lot! Really appreciate it! In a previous thread Junilu and Praveen and some other guys there told me that my code wasn't formatted well. So i went and read the page about code formatting and tried to improve and not make the same mistakes!
 
Liutauras Vilda
Marshal
Posts: 4483
304
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:I used isLess because the logic for that variable was "the first is less than the second" as you can see in the following lines (i presume you took this from my compareMagnitude() method).
Maybe then firstLessThanSecond instead of isLess? Because isLess could represent that second is less than first, you don't know that until you find relevant parts. But these are just mine thoughts, somebody else can have different opinion or think it is not important at all.
 
Junilu Lacar
Sheriff
Posts: 10948
158
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carlos Reves wrote:So i went and read the page about code formatting and tried to improve and not make the same mistakes!

That's what we call "showing some effort" around here and it seldom goes unnoticed or unrewarded. It's good to see you considering all the advice and feedback you've been given and putting it to good use. You'd be surprised how that doesn't happen as often as you'd think it would.
 
Liutauras Vilda
Marshal
Posts: 4483
304
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
earlier I wrote wrote:Maybe then firstLessThanSecond instead of isLess? Because isLess could represent that second is less than first, you don't know that until you find relevant parts.
See how much time we spend for one variable? It can take even longer, for me usually takes long until I find what I like. All those small details makes you feel good about what you have come up with. When you need to keep up with deadlines, you might not always have time to spend decent amount of time to think about all well, but when you come back to code with more time, you should aim to polish every corner you can. With current IDE's we have, you need just to come up with a good name, and to change it in all places in one go it just seconds, so it shouldn't be a scary task.
 
Knute Snortum
Sheriff
Posts: 3940
92
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The above method will allow you to set invalid signums.
 
Knute Snortum
Sheriff
Posts: 3940
92
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It will also allow an array with "bad" digits (>9).
 
Knute Snortum
Sheriff
Posts: 3940
92
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
...and with too big an array.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!