what is wrong with this Stream function ?

[tangara goh]

Hi guys,

I am lost on Java 8 Streams and it does not get easier with the way the question was asked in this exercise at

https://exercism.org/tracks/java/exercises/sum-of-multiples/edit

 
class SumOfMultiples {

    SumOfMultiples(int number, int[] set) {
        throw new UnsupportedOperationException("Delete this statement and write your own implementation.");
    }

    int getSum() {
        throw new UnsupportedOperationException("Delete this statement and write your own implementation.");
    }

}

//Given a number, find the sum of all the unique multiples of particular numbers up to
//but not including that number.
//
//If we list all the natural numbers below 20 that are multiples of 3 or 5,
//we get 3, 5, 6, 9, 10, 12, 15, and 18.
//
//The sum of these multiples is 78.

public class SumOfMultiples {

	private int num;
	private int[] arrM;


	SumOfMultiples(int number, int[] set) throws Exception {
		if(num < 0) {
			throw new UnsupportedOperationException("num cannot be less than 0");
		}
		if(set == null) {
			throw new UnsupportedOperationException("set cannot be empty");
		}
		this.num = number;
		this.arrM = set;
	}

	int getSum() throws Exception{
		int total = 0;	
		Set<Integer>multiples = new HashSet<Integer>();  
		for(int i : arrM)
			Stream.iterate(0, x -> x + 1).limit((num)).filter(x -> x%i == 0).forEach(x -> multiples.add(x));
		for(int y : multiples) {
			total += y;
		}
		System.out.println("what " + total);
		return total;
	}



	public static void main(String[] args) throws Exception {
		int number = 20;
		int[]set = new int[number];
		SumOfMultiples s = new SumOfMultiples(number, set); 	
		System.out.println(s.getSum()); 

	}
}

I hope someone could pinpoint the mistakes. Tks.

[Carey Brown]

Looks like set is never filled with anything so is all zeros.

What is the problem you are seeing?

[Campbell Ritchie]

tangara goh wrote:. . .//... if(set == null) { throw new UnsupportedOperationException("set cannot be empty"); } //...

That line is imprecise; you certainly don't want nulls, but please use a different exception (NullPointerException) and a more accurate error message. Yes, you do have to be very precise when writing code. I can't see an empty set being a problem; you will simply get an empty OptionalInt and you can use orElse(0). And I can't see why negative numbers would be a problem.
I would be wary of any site where they think set is a good name for an array.
I think you should go through the different methods of the IntStream interface. Start by writing down how you would add all the numbers in an array. Then work out how to remove duplicates or numbers greater than your limit.

. . . . Tks.

Please avoid abbreviations; I have seen Google translate struggle with “Tks”.
Also, use spaces for indenting rather than tabs, and please find out how to shorten long lines like line 11 there.

[tangara goh]

Carey Brown wrote:Looks like set is never filled with anything so is all zeros.

What is the problem you are seeing?

The problem is that it is returning zero since set is not returning anything.

How do i render it in the main void method ?

[Piet Souris]

hi Tangara,

there is no need to have an int[] parameter. Read the exercise again. So you woulg get something like

int sumOfMultiples(int n) {
    return IntStream.range(0, n).filter(i -> i %3 == 0 || i % 5== 0).sum();
}

[tangara goh]

Piet Souris wrote:hi Tangara,

there is no need to have an int[] parameter. Read the exercise again. So you woulg get something like

int sumOfMultiples(int n) {
    return IntStream.range(0, n).filter(i -> i %3 == 0 || i % 5== 0).sum();
}

That is what i thought, too.

But, in that platform, they have put int[]set as the parameters.

Hence, I am stumped and my heart filled with horror that I can't do anything with that int[]set inside as a parameter....

And there is no way to test out since that set is used to contain the unique number of mulitples.  Either I pre-generated it then i do that getSum using this pre-generated, doing 2 separate methods but then this is not given in the platform and all is given is that getSum method so I'm not sure if it should be done this way or wat ?

[Campbell Ritchie]

That won't return an int, Piet.
I missed the bit about dividing by 3 and 5. In which case, as you say, you don't need an array. You can use the method to create a stream of increasing numbers.

[Piet Souris]

Doesn't return an int? Admitted, I wrote it out of my head, but...

@Tangara
I don't see a method signature on the page you linked to, can you show us where you got that signature from?

[tangara goh]

Piet Souris wrote:hi Tangara,

there is no need to have an int[] parameter. Read the exercise again. So you woulg get something like

int sumOfMultiples(int n) {
    return IntStream.range(0, n).filter(i -> i %3 == 0 || i % 5== 0).sum();
}

why did you %3 and %5 when it should be i % number to get the multiples and then to use the set to filter the unique ones ?

[Piet Souris]

As I said, I could not find a method signature, and so I used 3 and 5 as in the example given.

But assuming the array is an array of possible divisors, in my example {3, 5}, you could have a helper method like:

private static boolean hasADivisor(int n, int[] possibleDivisors) {
    return Arrays.stream(possibleDivisors).anyMatch((i -> n % i == 0));
}

and so you could have this method

public static int sumOfMultiples(int n, int[] possibleDivisors) {
    return IntStream.range(1, n).filter(i -> ????).sum();
}

[Campbell Ritchie]

tangara goh wrote:. . . then to use the set . . .

What set? You haven't got a set anywhere. You only appear to have an array.

[tangara goh]

Piet Souris wrote:As I said, I could not find a method signature, and so I used 3 and 5 as in the example given.

But assuming the array is an array of possible divisors, in my example {3, 5}, you could have a helper method like:

private static boolean hasADivisor(int n, int[] possibleDivisors) {
    return Arrays.stream(possibleDivisors).anyMatch((i -> n % i == 0));
}

and so you could have this method

public static int sumOfMultiples(int n, int[] possibleDivisors) {
    return IntStream.range(1, n).filter(i -> ????).sum();
}

Sorry Piet, my bad... there is no parameter in the getSum() method at all.

It is my first time dealing with DSA combined with OOP so I am confused by that constructor what it is for.

But, really I don't understand why bind the set and the num at class level to the algorithm ?

[Liutauras Vilda]

@OP

So you just need to incorporate into the template you have been given a code Piet wrote.

What is DSA?


i.e.

public final class SumOfMultiples {

    private final int sum;

    SumOfMultiples(int number, int[] set) {
        this.sum = // call method demonstrated in this thread
    }

    int getSum() {
        return sum;
    }

    // declare methods demonstrated in this thread
}

Or

public final class SumOfMultiples {

    private final int number;
    private final int[] set;

    SumOfMultiples(int number, int[] set) {
        this.number = number;
        this.set = set;
    }

    int getSum() {
        // return by calling method demonstrated in this thread;
    }
    
   // declare methods demonstrated in this thread
}

Think which approach is better and why.

[Campbell Ritchie]

. . . and tell us why line 8 in Liutauras' second code block might cause problems.

[tangara goh]

Liutauras Vilda wrote:@OP

So you just need to incorporate into the template you have been given a code Piet wrote.

What is DSA?


i.e.

public final class SumOfMultiples {

    private final int sum;

    SumOfMultiples(int number, int[] set) {
        this.sum = // call method demonstrated in this thread
    }

    int getSum() {
        return sum;
    }

    // declare methods demonstrated in this thread
}

Or

public final class SumOfMultiples {

    private final int number;
    private final int[] set;

    SumOfMultiples(int number, int[] set) {
        this.number = number;
        this.set = set;
    }

    int getSum() {
        // return by calling method demonstrated in this thread;
    }
    
   // declare methods demonstrated in this thread
}

Think which approach is better and why.

If I were to use the first method, then what do i do in the main void method ?

public static void main(String[] args) throws Exception {
		int number = 20;
		//int[]set = new int[number]; 
		SumOfMultiples s = new SumOfMultiples(number, set); 	//what to do with a set here ?
	

cos I can't create a new int[number] and it is weird to make set static in order to bring it in....and it can't be static since the number length will change ?

[Carey Brown]

public final class SumOfMultiples {

    private final int sum;

    SumOfMultiples(int number, int[] set) {
        this.sum = calcSum( number, set );
    }

    int getSum() {
        return sum;
    }

    // declare methods demonstrated in this thread
    int calcSum( int number, int[] set )
    {
        //...
    }

    public static void main(String[] args) throws Exception {
	    SumOfMultiples s = new SumOfMultiples( 20, new int[]{3,5} );
	    System.out.println( s.getSum() );
    }
}

[tangara goh]

Carey Brown wrote:

public final class SumOfMultiples {

    private final int sum;

    SumOfMultiples(int number, int[] set) {
        this.sum = calcSum( number, set );
    }

    int getSum() {
        return sum;
    }

    // declare methods demonstrated in this thread
    int calcSum( int number, int[] set )
    {
        //...
    }

    public static void main(String[] args) throws Exception {
	    SumOfMultiples s = new SumOfMultiples( 20, new int[]{3,5} );
	    System.out.println( s.getSum() );
    }
}

I am sorry but I still don't see why the class is designed this way because we have to put in the new int[] {3,5} where actually this is not told to us, and then we have to pre-calculated the set array which is the number multiples ?

Maybe i am stupid, i still can't grasped why it is designed this way

[Piet Souris]

hi Tangara,

It is just one way to do it. What happens is that they create an instance of your class via the two-parameter constructor, and then call the getSum-metod on that instance. Since that method has no parameters, you have to make fields of the constructor-parameters, that you use in getSum.

You do not need a main-method. I just ran it, and got an error because my IntStream was unknown, so either you also must do the import, or you do not use an IntStream.

[Campbell Ritchie]

I think I would have used int... instead of int[], throughout, because that affords more flexibility in supplying arguments.
The reason the class was designed that way is so you can make it an object. That object encapsulates both data (the array, multiplier, and result) and behaviour (producing the result). It is possible to copy the array as a field, but that introduces potential pitfalls, which disappear if you use the array immediately. But maybe only in a singly‑threaded program.
Piet's problem about imports can be explained simply by saying it is included in or implied by the //... in line 16.

[Campbell Ritchie]

The method in lines 14‑17 is running as a pure function; it might beneficially have been made private static.

[Campbell Ritchie]

What I said in my last post suggests you can use that method in a functional style. But the example you showed in your first post has modifiers inconsistent with that. I don't like over‑specification like that, and my liking for that website you found has not increased.

[tangara goh]

Campbell Ritchie wrote:What I said in my last post suggests you can use that method in a functional style. But the example you showed in your first post has modifiers inconsistent with that. I don't like over‑specification like that, and my liking for that website you found has not increased.

Hmm...I thought that I was lucky to chance upon that site where they mixed OOP with DSA for a more complete educations.

What other sites are out there like codewar is free but with added OOP to the exercises ?

[Junilu Lacar]

The parameter int[] set is misleadingly named. It is actually the set of factors that you need to get multiples of.

You could have figured this out by looking at the test code:

@Test
public void testOneFactorIsAMultipleOfAnother() {
    int[] set = { 5, 25 };
    int output = new SumOfMultiples(51, set).getSum();
    assertEquals(275, output);
}

So the candidate numbers you need to sum are:

multiples of 5 that are less than 51:
5, 10, 15, 20, 25, 30, 35, 40, 45, 50

multiples of 25 that are less than 51:
25, 50

Since 25 and 50 are common to both sets of multiples, you only add them to the answer once. So basically, for this particular test, the answer would be the sum of all multiples of 5 that are less than 51 which is 275.

[Junilu Lacar]

One algorithm for a solution would be:
1. Iterate over the set of factors given (the second parameter, int[] set)
1a. For each factor, generate all the multiples of that factor that are less than the given number (the first parameter)
1b. Collect all the multiples generated in 1a, taking only unique values

2. Sum all the numbers collected in 1b.

[Campbell Ritchie]

tangara goh wrote:. . . I thought that I was lucky to chance upon that site . . .

Afraid I think you were rather unlucky. Don't know of any sites teaching OOP; sorry. Maybe somebody else will know.

[Junilu Lacar]

This passes all tests:

import java.util.*;
import java.util.stream.*;

class SumOfMultiples {

    int number;
    int[] factors;
    
    SumOfMultiples(int number, int[] set) {
        this.number = number;
        this.factors = set;
    }
    
    private IntStream multiplesOf(int factor, int limit) {
        return IntStream.iterate(factor, n -> n + factor)
            .limit(limit/factor);
    }
    
    int getSum() {
        return Arrays.stream(factors)
            .filter(factor -> factor != 0)
            .flatMap(factor -> multiplesOf(factor, number - 1))
            .distinct()
            .sum();
    }

}

Maybe there's a shorter/better way to do it... ?

[Junilu Lacar]

tangara goh wrote:I hope someone could pinpoint the mistakes. Tks.

Your code fails two tests, both with a division by zero exception.

@Test
public void testSumOfMultiplesOfZeroIsZero() {
    int[] set = { 0 };
    int output = new SumOfMultiples(1, set).getSum();
    assertEquals(0, output);
}

@Test
public void testFactorZeroDoesNotAffectTheSumOfMultiplesOfOtherFactors() {
    int[] set = { 3, 0 };
    int output = new SumOfMultiples(4, set).getSum();
    assertEquals(3, output);
}

[Campbell Ritchie]

Let's try multiplying all numbers up to a limit, by a multiplier, and excluding numbers divisible by two factors.

int sum = IntStream.range(0, limit)
                   .filter (i -> i % factor1 != 0 && i % factor2 != 0)
                   .map(i -> i * multiplier)
                   .sum()
                   .orElse(0);

Line 1: Declares the result. Creates an IntStream iterating 0...limitexclusive. If limit < 1 we get sum = 0. If limit = 20 we get 20 numbers, the largest being 19.

Line 2: Creates a second IntStream by filtering in (retaining) numbers not dividing exactly by factor1 and by factor2. If either factor is 0, the division will throw an exception. If either factor has an absolute value greater than or equal to limit, that test with % will never produce 0.

Line 3: Produces a third IntStream whose elements are those of its predecessor multiplied by something. There is a risk of arithmetic overflow here or in the next stage if the numbers are large enough.

Line 4: Collects the sum as an OptionalInt.

Line 5: If any Stream was empty, you will have an empty OptionalInt, in which case you can convert that to 0 with its  orElse() method.

[Junilu Lacar]

This way also passes all 16 tests:

import java.util.*;
import java.util.stream.*;

class SumOfMultiples {
    int max;
    int[] factors;
    
    SumOfMultiples(int number, int[] set) {
        max = number;
        factors = set;
    }
    
    private boolean isMultipleOfAny(int n, int[] factors) {
        return Arrays.stream(factors)
            .filter(f -> f != 0)
            .anyMatch(d -> n % d == 0);
    }
    
    int getSum() {
        return IntStream
            .iterate(1, n -> n + 1)
            .limit(max - 1)
            .filter(n -> isMultipleOfAny(n, factors))
            .sum();
    }
}

Note that the filter to guard against division by zero is needed because there are test cases that include 0 in the second parameter.

The first approach I shared generated all the multiples of the numbers given in the array of factors (that's what they're referred to in the problem and tests).

This second approach checks if a number is a multiple of any of the factors.

Either way works. Which one saves you a few cycles depends on the values that are passed in. I haven't spent any time analyzing the performance of each algorithm though.

[Carey Brown]

Why not

IntStream.range(1,max)

?

[Junilu Lacar]

Carey Brown wrote:Why not

IntStream.range(1,max)

?

Because the problem specifies "up to but not including the number"

[Carey Brown]

static IntStream range(int startInclusive, int endExclusive)

from javadocs

[Junilu Lacar]

You're right, this works, too:

    int getSum() {
        return IntStream
            .range(1, max)
            .filter(n -> isMultipleOfAny(n, factors))
            .sum();
    }