Factorial method bug

Hi,

I've written 2 methods that supposed to return eventually the sum of factorials in an array (every element consists only 1 digit).

After checking it time after time and going throught it to figure what's wrong, I've realized that after the first digit the method calculates, it's just taking every following digit and returns "1" as it's factorial value.

I just can't figure out why! I've copied it so u can have a look:

Code:

---

public static int sumFactorial (int[] B , int numElementsB) { //sumFactorial method starts
                int sumFactorial=0;
                for (int i=1 ; i < numElementsB ; i++ ) {
                       
                        if (B[i] <= 1 ) sumFactorial++;
                        else sumFactorial+=factorial(B[i]);               
                }//for ends
               
                return sumFactorial;
               
               
        }
       
        public static int factorial (int num) { //factorial method starts
                int facNum=1;
                for (int i = 1 ; i <= num ; i++){
                        facNum=(facNum*i);
                }//for ends
                return facNum;
               
               
               
        }

---

It works for me. Ie the following code prints 9 as expected.

Code:

---

public class FacTest {       
        public static void main(String args[]) {
                int[] arr = new int[] {0, 1, 2, 3, 2, 1};
                int num = 4;
               
                        // should print 9 ie 1!+2!+3!
                System.out.println(sumFactorial(arr, num));
        }

                /**
                * Returns the sum of the factorials of a given int array from index 1 to
                * just before a given index.
                *
                * @param  B            the array to be summed
                * @param  numElementsB  the index just after where the summation ends
                */
        public static int sumFactorial (int[] B , int numElementsB) {
                int sumFactorial=0;
               
                for (int i=1 ; i < numElementsB ; i++ ) {
                        if (B[i] <= 1 ) sumFactorial++;
                        else sumFactorial+=factorial(B[i]);   
                }
                return sumFactorial;
        }

        public static int factorial (int num) {
                int facNum = 1;
               
                for (int i = 1 ; i <= num ; i++){
                        facNum=(facNum*i);
                }
                return facNum;
        }
}

---

Note the comment on the sumFactorial() method. Perhaps you mean it to do something different?

In particular if you want numElementsB factorials to be summed (rather than numElementsB-1 of them) you had better change the for loop in sumFactorial().

when i try it with the array {3,3}
it gives me as factorial sum 7 instead of 12

in the first element it does it correct, on the second, third, etc it just gives "1" instead of calculating it.

I've tried changing the loop index to be until numElementB but it didn't help :(

It's hard to tell why you are getting 7 without seeing the code... As I said the code you posted does exactly what it should: it sums the factorials for a portion of the array it is passed. If you are seeing something else, that's because of other code.

hmm.. i see..
I haven't found anything myself, that's why i posted the thread.
I'll try again to go over all the code, then if i haven't found the problem I'll probably post more of the code.
Now at least i know where the problem doesn't exist.

Tnx again :)

In the for loop of the sumFactorial() method try printing out the values of B[i] at the start. Perhaps the array does not contain the values you think, or they are not in the places you think they are. 7=3!+0! so it may be that you are looking at the array values one place "shifted".

Quote:

---

Originally Posted by Mapisto

hmm.. i see..
I haven't found anything myself, that's why i posted the thread.
I'll try again to go over all the code, then if i haven't found the problem I'll probably post more of the code.
Now at least i know where the problem doesn't exist.

Tnx again :)

---

Double check that you have saved and compiled the version you have posted.

If you compile without saving your latest changes, or you save without compiling, it can be very frustrating!!!

Oh hell :|

I believe I've found my problem!

In my last thread you've told me all that thing with the method comparing it to an orange seller

Well, if what you say it's true (and of course it is!) then i've changed the array with another method before it got to the factorial method! That's because i had no idea it changed the array in the main :(

now I'll have to think how to fix this........ unending problems :( I'm sure he thought about all this when he wrote this exercise.......................................... .....................

guys, take a look at this method:

Code:

---

public static int differentDigits (int[] B , int numElementsB) { //differentDigits method starts
               
                int count = 0;
                for (int i = 1 ; i < numElementsB ; i++) {
                        if (B[i]>=0) count++;
                        for (int n = (i+1) ; n < numElementsB ; n++){
                                       
                                        if (B[i]==B[n]) {
                                                B[n] =-1;
                                        }//2st if ends
                               
                               
                        }//2st for ends
                       
                       
                       
                }//1st for ends
               
                return count;

---

This one is supposed (and does) return how many different digits are in a given array.

Well, u can see from the way I've written it that i changed the array completely without knowing it will affact the array in the main method...

now I have 2 options:
option 1: change the method in a way that it does not change the array.
option 2: find a way to save the same array without the changes b4 invoking the method, and then give it back after invoking it.

Now when i think of it, option 2 won't be available.

There are given lines to the program that I must not change......... all must happen inside the method.


edit:
I could just create a new array in the method and be4 it starts changing the original array, copying its values to the new 1.
in the end, I could just insert all the saved values to the array that has been changed.

But this way seems barbaric to me :| there must be an other way to do it without creating a new array with 1000 elements that takes so much memory!


edit:

I've tried it after all, though it didn't work and i wonder why:

Code:

---

public static int differentDigits (int[] B , int numElementsB) { //differentDigits method starts
               
                int[] C = new int[1000];
                int count = 0;
                C=B;
                for (int i = 1 ; i < numElementsB ; i++) {
                        if (B[i]>=0) count++;
                        for (int n = (i+1) ; n < numElementsB ; n++){
                                       
                                        if (B[i]==B[n]) {
                                                B[n] =-1;
                                        }//2st if ends
                               
                               
                        }//2st for ends
                       
                       
                       
                }//1st for ends
                B=C;
                return count;
               
               
               
        }

---

Your original code was nearly there. It doesn't write into B[]. Did you see pbrockway2's comment at the end?

Quote:

---

Originally Posted by pbrockway2

It works for me. Ie the following code prints 9 as expected.

Code:

---

public class FacTest {       
        public static void main(String args[]) {
                int[] arr = new int[] {0, 1, 2, 3, 2, 1};
                int num = 4;
               
                        // should print 9 ie 1!+2!+3!
                System.out.println(sumFactorial(arr, num));
        }

                /**
                * Returns the sum of the factorials of a given int array from index 1 to
                * just before a given index.
                *
                * @param  B            the array to be summed
                * @param  numElementsB  the index just after where the summation ends
                */
        public static int sumFactorial (int[] B , int numElementsB) {
                int sumFactorial=0;
               
                for (int i=1 ; i < numElementsB ; i++ ) {
                        if (B[i] <= 1 ) sumFactorial++;
                        else sumFactorial+=factorial(B[i]);   
                }
                return sumFactorial;
        }

        public static int factorial (int num) {
                int facNum = 1;
               
                for (int i = 1 ; i <= num ; i++){
                        facNum=(facNum*i);
                }
                return facNum;
        }
}

---

Note the comment on the sumFactorial() method. Perhaps you mean it to do something different?

In particular if you want numElementsB factorials to be summed (rather than numElementsB-1 of them) you had better change the for loop in sumFactorial().

---

yea, I've read it and he's right, but it still doesn't solve the main problem.

Can't i just copy 2 arrays like i did ? or do i need to refer into each value seperately with a new loop?

Do you think you have a problem because of this test? What did you use for numElementsB in this test?

Quote:

---

Originally Posted by Mapisto

when i try it with the array {3,3}
it gives me as factorial sum 7 instead of 12

in the first element it does it correct, on the second, third, etc it just gives "1" instead of calculating it.

I've tried changing the loop index to be until numElementB but it didn't help :(

---

tbh I can't see how this gave you 7 unless you've been testing something like this {3,3,0}

yea, it's the number i've checked.

you see, i wrote this method that every time it sees a number that repeats itself, it makes him become (-1).
Then i didn't know the method affects the array also in the main, now that i know that it does, i know what happend.
when the sumfactorial method recieves the array (after it has been touched by differentDigits method) it contain (-1) in most of its elements.

what i need is a way to make array B stay the same.
I've found a way to do so (which i don't really like coz it takes a lot of memory), but it won't work.
I've posted it up, so u can see. i guess i need a new loop in order to get B elements values into a new C array : \ is that correct?

Ok,
look what I've done now:

Code:

---

public static int differentDigits (int[] B , int numElementsB) { //differentDigits method starts
               
                int[] C = new int[1000];
                BintoC(B , C , numElementsB );
                int count = 0;
                for (int i = 1 ; i < numElementsB ; i++) {
                       
                        if (C[i]>=0) count++;
                        for (int n = (i+1) ; n < numElementsB ; n++){               
                                        if (C[i]==C[n]) {
                                                C[n] =-1;
                                        }//2st if ends
                               
                               
                        }//2st for ends
                       
                       
                       
                }//1st for ends
                return count;
               
               
               
        }


        public static void BintoC (int[] B , int[] C, int numElementsB){ // BintoC method starts - copying B elements values into C.
                for (int i=0 ; i <= numElementsB ; i++){
                        C[i]=B[i];
                }
               
               
        }

---

Well, it works fine now, but it still annoys me.
Isn't there any other way of copying this array into the other without invoking a new method and a loop?! (i can't put 2 loops in the same method (except differentDigits, only there I'm alowed)).

Or even better, a way of doing this without creating a new array for it :|

I'd love to get some ideas.. meanwhile I'm gonna smash my head at the wall a little until something comes up :frusty:

Original code (with correction as specified by pbrockway2, works fine on my machine. {3, 3} gives me 12.

Brrrr...I had to type it all in by hand for reasons to complex to get into..

A few things:

If you are going to do multiple things with the array it is (as you have found) really, really important that you don't alter the array contents. The array - to use Barbara Liskov's term - is a "shared object". It is shared between the caller and the methods. That's what makes the "return" unnecessary in the other thread. And, as I think you have also seen, it means that the methods "can look, but must not touch" when it comes to the array.

I agree with you that making a copy of the array in the digit counting method will work, but it feels wrong. What about this: the differentDigits() method could create a small "counter" array, just 10 int elements wrong. Every time it sees a digit in the big array it increases the appropriate element in the counter array. Then to report how many different digits there are it need only count how many nonzero digits there are in the counter array. Since what you have works you might want to put this aside until later.

When you say that your code gives the "wrong" value, you seem to be basing this on what result you get after all sorts of other methods have been at work. Clearly neither me nor 2x4 nor anybody else here knows what those other methods are. A better approach (which I tried to hint at in my first reply) is to make the methods self-contained so each can be tested and verified on it's own. And document what the method in question is supposed to do - for your benefit as well as ours.

This approach will not make bugs go away. But it might shine light on them. In particular it might distinguish between whether a problem arises because the a method is faulty and whether the problem is that some other method has unwanted side effects.

You say "it works now". Does that mean the digits method? the factorial? both?

Quote:

---

Originally Posted by 2by4

Original code (with correction as specified by pbrockway2, works fine on my machine. {3, 3} gives me 12.

Brrrr...I had to type it all in by hand for reasons to complex to get into..

---

Thanks man :|

Quote:

---

Originally Posted by pbrockway2

A few things:

If you are going to do multiple things with the array it is (as you have found) really, really important that you don't alter the array contents. The array - to use Barbara Liskov's term - is a "shared object". It is shared between the caller and the methods. That's what makes the "return" unnecessary in the other thread. And, as I think you have also seen, it means that the methods "can look, but must not touch" when it comes to the array.

I agree with you that making a copy of the array in the digit counting method will work, but it feels wrong. What about this: the differentDigits() method could create a small "counter" array, just 10 int elements wrong. Every time it sees a digit in the big array it increases the appropriate element in the counter array. Then to report how many different digits there are it need only count how many nonzero digits there are in the counter array. Since what you have works you might want to put this aside until later.

When you say that your code gives the "wrong" value, you seem to be basing this on what result you get after all sorts of other methods have been at work. Clearly neither me nor 2x4 nor anybody else here knows what those other methods are. A better approach (which I tried to hint at in my first reply) is to make the methods self-contained so each can be tested and verified on it's own. And document what the method in question is supposed to do - for your benefit as well as ours.

This approach will not make bugs go away. But it might shine light on them. In particular it might distinguish between whether a problem arises because the a method is faulty and whether the problem is that some other method has unwanted side effects.

You say "it works now". Does that mean the digits method? the factorial? both?

---

It works fine now, all of it.
I wish i just knew from the beginning about methods changing arrays and all this stuff.. could have saved me a lot of trouble.. but that's what i get for making shortcuts.. i learn the hard way :|
Now all i want to try and do is to simplify it and make it shorter.

I'll also remember your comment about saparating methods and check them (or post questions about them) individually.

Thanks again.

You're welcome.