for loop problem
i want this code to print out amicable numbers below 300, the output should therefore be:
"220,284 are amicable numbers."
this is because the sum of the divisors of 220 = 284, and the sum of the divisors of 284 = 220.
But the output prints many combinations of numbers, but there should only be 220 + 284 like i said,
I don't know if the problem is with the for loop in the main method or the loops in the test method, or both. I want the for loop in the main method to compare all the numbers from 1 to 300, then 2 to 300, then 3 to 300 etc.
can anyone show me what i've done wrong?
class AmicableNumbers{
publicstaticvoid main(String[] args){
for(int num = 1; num < 299; num++){
for(int numx = num+1; numx < 300; numx++){
if( test(num,numx) )
System.out.println(num+" and "+numx+" are amicable numbers");
}
}
}//close main.
staticboolean test(int num1,int num2){
int p = 0, p2 = 0, sum = 0, sum2 = 0, elements = 0, elementsTwo = 0;
for(int y = 1; y < num1; y++){
if(num1 % y == 0) elements +=1;
}
int[] factors =newint[elements];
for(int y = 1; y < factors.length; y++){
if(num1 % y == 0) factors[p++] = y;
}
for(int y = 0; y < factors.length; y++){
sum = sum + factors[y];
}
//num2
for(int y = 1; y < num2; y++){
if(num2 % y == 0) elementsTwo +=1;
}
int[] factorsTwo =newint[elementsTwo];
for(int y = 1; y < factorsTwo.length; y++){
if(num2 % y == 0) factorsTwo[p2++] = y;
}
for(int y = 0; y < factorsTwo.length; y++){
sum2 = sum2 + factorsTwo[y];
}
if(sum == sum2)returntrue;
elsereturnfalse;
}
}
[3876 byte] By [
mark_8206a] at [2007-11-27 0:50:51]
The way you are looking for divisors is incorect.
for(int y = 1; y < factors.length; y++) {
for(int y = 1; y < factorsTwo.length; y++) {
These are wrong lines...
Final check (sum==sum2) is wrong also.
// find number of factors (n)
for(int y = 1; y < num1; y++) {
if(num1 % y == 0) elements +=1;
}
int[] factors = new int[elements];
// suppose to put all the factors in array
// but you actually put 1,2,...n
for(int y = 1; y < factors.length; y++) {
if(num1 % y == 0) factors[p++] = y;
}
// sum all the factors
for(int y = 0; y < factors.length; y++) {
sum = sum + factors[y];
}
You should rewrite the 2nd for loop
To improve the performance, you should get all the sum of the factors of numbers first, then find a pair. You have do too much duplicate jobs.
You should use ArrayList if you are using JDK v1.5+ or may be Vector.
rym82a at 2007-7-11 23:21:22 >
> can anyone show me what i've done wrong?
In my opinion you should split up the functionality more. That way errors are easier to detect. Example:
class Test {
public static void main(String args[]) {
int max = 300;
for (int i = 1; i < max; i++) {
for (int j = i + 1; j < max; j++) {
if (areAmicablePair(i, j)) {
System.out.println("The numbers " + i + " and " + j + " are an amicable pair");
}
}
}
}
static boolean areAmicablePair(int number, int anotherNumber) {
if ((sumOfProperDivisors(number) == anotherNumber) && (sumOfProperDivisors(anotherNumber) == number)) {
return true;
}
return false;
}
static int sumOfProperDivisors(int number) {
int sum = 0;
for (int divisor = 1; divisor < number; divisor++) {
if (divides(divisor, number)) {
sum += divisor;
}
}
return sum;
}
static boolean divides(int divider, int number) {
return ((number % divider) == 0);
}
}
Ah, the man of the main-method! Oh, wait: you have a single method I see: things are improving!
; )
Couple of things: please use some proper indentation: your code is hard to follow with everything aligned with one tab.
Your method should be split into more parts. My suggestion is to do it like this:
class AmicableNumbers {
/**
* @param minteger value to check with 'n' if they're amicable
* @param ninteger value to check with 'm' if they're amicable
* @returntrue if 'm' and 'n' are amicable, else false
*/
public static boolean areAmicable(int m, int n) {
int[] divisorsM = getProperDivisors(m);
int[] divisorsN = getProperDivisors(n);
int sumDivisorsM = sum(divisorsM);
int sumDivisorsN = sum(divisorsN);
return sumDivisorsM == n && sumDivisorsN == m;
}
/**
* @param nan integer value
* @returnan array of all integers which divide 'n' (not 'n' itself)
*/
public static int[] getProperDivisors(int n) {
// your code
}
/**
* @param arrayan array of integer values
* @return the sum of all the integers present in 'array'
*/
public static int sum(int[] array) {
// your code
}
/**
* main method
* @param args
*/
public static void main(String[] args) {
for(int num = 1; num < 299; num++) {
for(int numx = num+1; numx < 300; numx++) {
if(areAmicable(num,numx)) {
System.out.println(num+" and "+numx+" are amicable numbers");
}
}
}
}
}
I have said this numerous times to you, but I'll say it again:
- do not stuff so much responsibility in ONE method;
- use proper variable names and method names;
- use System.out.println()'s in your code to debug it to see why numbers are being accounted for as amicable;
- no, I will not look at your current code, sorry.
Message was edited by:
prometheuzz
Changed "distinct" --> "proper" after reading EvilBro's post: I mixed things up again!
; )
> > can anyone show me what i've done wrong?
> In my opinion you should split up the functionality
> more. That way errors are easier to detect.
The OP is reluctant to do such things. The OP has also been told numerous times to debug that mess of his by putting System.out.println()'s inside hisher code, but until now: alas, s/he keeps ignoring the advice.
> The OP is reluctant to do such things.
Has the OP already been called stupid to see if that gets the message across?
> ... but until now: alas, s/he keeps ignoring the advice.
Probability-wise I think you can safely go with 'he' given the nick 'mark' (unless the OP tries to be the mark of something of course :) ).
> > > can anyone show me what i've done wrong?
> > In my opinion you should split up the
> functionality
> > more. That way errors are easier to detect.
>
> The OP is reluctant to do such things. The OP has
> also been told numerous times to debug that mess of
> his by putting System.out.println()'s inside hisher
> code, but until now: alas, s/he keeps ignoring the
> advice.
i changed the for loops in my code from
for(int y = 1; y < factors.length; y++) {
to
for(int y = 1; y < num1; y++) {
but i still get the same problem, where would i put the system.out.println's to show me what's going wrong?
> Probability-wise I think you can safely go with 'he' given the nick 'mark'In my country there are many names with short forms suitable for both men and women :)Possible OP from country there 'mark' is name for women...
> but i still get the same problemRead my answer again.
> ...
> Has the OP already been called stupid to see if that
> gets the message across?
No, I wouldn't go that far: he's been a decent poster here (using code tags, explaining his problem, etc).
I'm just a bit amazed.
> Probability-wise I think you can safely go with 'he'
> given the nick 'mark' (unless the OP tries to be the
> mark of something of course :) ).
Yes, you're probably right. I'm a bit numb today!
; )
i changed the for loops in my code from
for(int y = 1; y < factors.length; y++) {
to
for(int y = 1; y < num1; y++) {
but i still get the same problem, where would i put the system.out.println's to show me what's going wrong?
Why do you change it ? Why do you change it like that ?
What do you really need ?
Think about it before you modify it. Don't just make random guess.
rym82a at 2007-7-11 23:21:22 >
> ...
> but i still get the same problem, where would i put
> the system.out.println's to show me what's going
> wrong?
Wherever you like in your code! I have explained this a couple of times to you!
But why not split things up in a couple of decent methods?
Anyway, here's how you can strategically place System.out.println()'s inside your methods to see what's happening:
class AmicableNumbers {
public static void main(String[] args) {
/*
for(int num = 1; num < 299; num++) {
for(int numx = num+1; numx < 300; numx++) {
if( test(num,numx) ) {
System.out.println(num+" and "+numx+" are amicable numbers");
}
}
}
*/
int m = 94, n = 106;
if( test(m,n) ) {
System.out.println(m+" and "+n+" are amicable numbers");
}
}
static boolean test(int num1, int num2) {
int p = 0, p2 = 0, sum = 0, sum2 = 0, elements = 0, elementsTwo = 0;
for(int y = 1; y < num1; y++) {
if(num1 % y == 0) elements +=1;
}
int[] factors = new int[elements];
for(int y = 1; y < factors.length; y++) {
if(num1 % y == 0) factors[p++] = y;
}
for(int y = 0; y < factors.length; y++) {
sum = sum + factors[y];
}
System.out.println(num1+" factors: "+java.util.Arrays.toString(factors));
for(int y = 1; y < num2; y++) {
if(num2 % y == 0) elementsTwo +=1;
}
int[] factorsTwo = new int[elementsTwo];
for(int y = 1; y < factorsTwo.length; y++) {
if(num2 % y == 0) factorsTwo[p2++] = y;
}
for(int y = 0; y < factorsTwo.length; y++) {
sum2 = sum2 + factorsTwo[y];
}
System.out.println(num2+" factors: "+java.util.Arrays.toString(factorsTwo));
System.out.println("\nsum 1 = "+sum);
System.out.println("sum 2 = "+sum2);
if(sum == sum2) return true;
else return false;
}
}
You could of course also use a debugger and place appropriate breakpoints... that way you don't even have to change your code (although you should really change your code. Go by this motto: if it isn't obvious (within reason) what your code does, change it.).
> > ... but until now: alas, s/he keeps ignoring the advice.
> Probability-wise I think you can safely go with 'he' given the nick 'mark'
> (unless the OP tries to be the mark of something of course :) ).
men ... *sigh*
kind regards,
Jethro (Mrs ;-)
JosAHa at 2007-7-11 23:21:22 >
> In my country there are many names with short forms
> suitable for both men and women :)
> Possible OP from country there 'mark' is name for
> women...
in botswana mark is name of my pet rabbit. but he not yet work out how use keyboord so i not think question from him. rember thsi advices.
> > > ... but until now: alas, s/he keeps ignoring the
> advice.
>
> > Probability-wise I think you can safely go with
> 'he' given the nick 'mark'
> > (unless the OP tries to be the mark of something of
> course :) ).
>
> men ... *sigh*
>
> kind regards,
>
> Jethro (Mrs ;-)
ok this has went on way too long!
i'm male!:) and i've never known a woman called Mark!
anyway, i still haven't worked out what i've **** wrong, but give me time.
or maybe you could actually just tell me which would be great!
> ...
> anyway, i still haven't worked out what i've ****
> wrong, but give me time.
> or maybe you could actually just tell me which would
> be great!
See my post how I showed how to use System.out.println()'s (also called the poor man's debugger). You'll see what happens.
>
> See my post how I showed how to use
> System.out.println()'s (also called the poor man's
> debugger). You'll see what happens.
Prome i'm getting errors with that, probably my old jdk version.
AmicableNumbers.java:37: toString() in java.lang.Object cannot be applied to (int[])
System.out.println(num1+" factors: "+java.util.Arrays.toString(factors));
^
AmicableNumbers.java:53: toString() in java.lang.Object cannot be applied to (int[])
System.out.println(num2+" factors: "+java.util.Arrays.toString(factorsTwo));
^
2 errors
> anyway, i still haven't worked out what i've **** wrong,
You think "done" is a dirty word? :)
> or maybe you could actually just tell me which would be great!
Take a look at the code you've been presented. Notice the structure and readability. Try implementing such structure and readability into your own code and you'll find your error will either go away on its own, or it will present itself more clearly... either way you'll win.
> ...
> AmicableNumbers.java:53: toString() in
> java.lang.Object cannot be applied to (int[])
> System.out.println(num2+" factors:
> "+java.util.Arrays.toString(factorsTwo));
>
>
> 2 errors
Apparently you use an older version of the JDK (1.4.2 or less), which doesn't have the java.util.Arrays.toString(int[]) method. Replace it with your own code that'll print the elements from your factors-arrays.
> ...> You think "done" is a dirty word? :)> No, the OP wrote dong, which is another word for a dildo or a penis.
> notice the smiley....Like I said: I'm pretty numb today.; )
> > ...
> > You think "done" is a dirty word? :)
> >
>
> No, the OP wrote ****, which is another
> word for a ***** or a penis.
The Penis Song
"Isn't it awfully nice to have a penis
Isn't it frightfully good to have a ****
It's swell to have a stiffy
It's divine to own a ****
From the tiniest little tadger
To the world's biggest *****
So three cheers for your willy or John Thomas
Hooray for your one-eyed trouser snake
Your piece of pork, your wife's best friend
Your Percy or your ****
You can wrap it up in ribbons
You can slip it in your sock
But don't take it out in public
Or they will put you in the dock
And you won't come back"
kind regards,
Jos ;-)
JosAHa at 2007-7-21 19:53:37 >
For the OP: I thought I'd just demonstrate the benefit of writing clear code. Suppose you've written what I've written before and you now want to optimize a bit because you want to extend the range in which to find numbers. You could simply do the following:
class Test {
public static void main(String args[]) {
Test tester = new Test();
tester.run(10000);
}
int[] sumOfProperDivisorTable;
void run(int max) {
System.out.println("Setting up table");
createLookUpTable(max);
System.out.println("Searching...");
for (int i = 1; i < max; i++) {
for (int j = i + 1; j < max; j++) {
if (areAmicablePair(i, j)) {
System.out.println("The numbers " + i + " and " + j + " are an amicable pair");
}
}
}
System.out.println("That's all I could find up in the range [0," + max + "].");
}
void createLookUpTable(int max) {
this.sumOfProperDivisorTable = new int[max + 1];
for (int i = 1; i < max + 1; i++) {
this.sumOfProperDivisorTable[i] = sumOfProperDivisors(i);
}
}
boolean areAmicablePair(int number, int anotherNumber) {
//if ((sumOfProperDivisors(number) == anotherNumber) && (sumOfProperDivisors(anotherNumber) == number)) {
if ((this.sumOfProperDivisorTable[number] == anotherNumber) && (this.sumOfProperDivisorTable[anotherNumber] == number)) {
return true;
}
return false;
}
int sumOfProperDivisors(int number) {
int sum = 0;
for (int divisor = 1; divisor < number; divisor++) {
if (divides(divisor, number)) {
sum += divisor;
}
}
return sum;
}
boolean divides(int divider, int number) {
return ((number % divider) == 0);
}
}
As you can see, the changes are minimal. The performance gain is significant. The resulting code is still clear. And all because you started out with clear code to begin with.
> thanks evilNot that you'll listen to his good advice. I bet your next post here is not different from what you have produced so far.Good luck anyway.
with regards to my code in the first post, even if the layout is bad, is it only the for loops in the test method that need to change? are the loops in the main ok?
> with regards to my code in the first post, even if
> the layout is bad, is it only the for loops in the
> test method that need to change? are the loops in the
> main ok?
May I suggest debugging your code: in other words: place some System.out.println()'s in your code so you see yourself what's happening.
