2
\$\begingroup\$

I was wondering if this code could be optimized in any way? For instance, could I use a for each loop here? Are there other ways I could write this which would make it easier for me, or the person reading it to understand?

public static void printFactors(int factorsOf) {
    for (int i = 1; i < factorsOf; i++) {
        if(factorsOf % i == 0) {
            System.out.print(i + " and ");
        }
    }
    System.out.print(factorsOf);
}
\$\endgroup\$
4
\$\begingroup\$

Using a print in each loop will be slow. There's too many of them, and print's are inherently slow because they are IO bound, require locks and synchronization, etc.

Additionally, looping for each possible factor is slow too. There are faster ways to identify the factors of a number, that's a whole other discussion though (Wikipedia has a great page).

So, without altering your algorithm, I would still make changes.

The first thing I would do is break out a function to compute an int-array of the factors. It would look something like:

public static final int[] factors(final int factorsOf) {
    return IntStream.rangeClosed(1, factorsOf)
            .filter(f -> factorsOf % f == 0)
            .toArray();
}

That's Java 8 streams, but the logic should be relatively clear. Loop from 1 to factorsOf, keep those values which are actual factors, and convert the factors to an array of int[].

That is a single-purpose function, which makes it clear, and reusable.

Now, create a function that converts an integer array to an "and" separated string... something like:

public static final String joinInts(int[] values, String joiner) {
    StringJoiner sj = new StringJoiner(joiner);
    for (int v : values) {
        sj.add(Integer.toString(v));
    }
    return sj.toString();
}

Then, use that all as:

    System.out.println(joinInts(factors(100), " and "));
\$\endgroup\$
  • \$\begingroup\$ Using streams has any performance benefits? \$\endgroup\$ – CodeYogi Apr 28 '16 at 16:35
5
\$\begingroup\$

Just a tiny note:

In number theory, integer factorization is the decomposition of a composite number into a product of smaller integers.

So a factorization of 12 may be

6 * 2

or

3 * 2 * 2

but not

1 and 2 and 3 and 4 and 6 and 12

What you did is finding all divisors and you should name your method appropriately.


As always, a method both computing and printing something is rather unusable. You should always compute the result in one method, so that you can later either print it or process it further.

For simple methods like yours it doesn't matter much, but it's good habit.

\$\endgroup\$
2
\$\begingroup\$

Once you find that i is a factor, you know that factorsOf / i is also a factor. This means that the for loop doesn't need to exceed the square root of factorsOf. For example, with 75, you will immediately get complementary factors of 75, 25, and 15 for the early factors of 1, 3, and 5. There is no point in checking for values of i above 9 (slightly higher than sqrt(75)). This will save a great deal of testing for large values of factorsOf since 1 through sqrt(n) has far fewer elements than sqrt(n) through n. If you need all the factors in order, you could store the early factors and their complementary factors in an array and sort it later.

\$\endgroup\$
0
\$\begingroup\$

If you are asking for all factors of the given number this can be done in Sqrt(n). Divisors comes in pair if i divides n then n/i too divides n hens it can be done on sqrt(n) Below is the cpp inplementation.

#include <bits/stdc++.h>
using namespace std;


vector<int> all_factor(int n){
    vector<int> factor;
    for(int i=1;i<=sqrt(n);i++){
        if(n%i==0){
            factor.push_back(i);//if i divids n so n/i too divids n
            if(n/i!=i)factor.push_back(n/i);
        }
    }
    return factor;
}
int main() {

    vector<int> ans = all_factor(6000);
    sort(ans.begin(),ans.end());
    for(int i=0;i<ans.size();i++)cout << ans[i]<<",";
}
\$\endgroup\$
0
\$\begingroup\$

I like @rolfl stream bases factors implementation but don't like much his joinInts method. The both tasks - finding factors and creating text output - may be implemented only with a help of Stream API only:

public static void printFactors(int factorsOf) {
    String result =
        IntStream.rangeClosed(1, factorsOf)
            .filter(f -> factorsOf % f == 0)
            .mapToObj(v -> Integer.toString(v))
            .collect(Collectors.joining(" and "));

    System.out.println(result);
}    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.