4
\$\begingroup\$

I have written a solution to find the occurrence of a common number in a n lists. I am just concerned if this is the best solution? Please suggest the best way to do it.

import java.util.Arrays;
import java.util.Collections;
import java.util.List;

public class CommonNumberFinder
{

    final static List<Number> list1 = Arrays.asList(new Number[] { 1, 2, 3, 4, 5, 6, 7, 8, 9 });
    final static List<Number> list2 = Arrays
            .asList(new Number[] { 3, 5, 1, 6, 2, 6, 3, 3, 6, 1, 5, 7, 2, 6, 2, 5, 2, 6, 8, 2 });

    public static void main(String[] args)
    {
        findCountOfNumber(Arrays.asList(new List[] { list1, list2 }), 6);
    }

    private static void findCountOfNumber(final List<List<Number>> lists, final Number num)
    {
        int count = 0;
        for (List<Number> list : lists)
        {
            if (!list.contains(num))
            {
                System.err.printf("Number %d is not common", num);
                return;
            }
        }
        for (List<Number> list : lists)
        {
            for (Number number : list)
            {
                if (number == num)
                    count++;
            }
            System.out.println("List have-" + count);
            count = 0;
        }

    }
}
\$\endgroup\$
9
\$\begingroup\$

There are a decent number of reservations I take with this code. Most IDEs would pick this up, but there are some nuances which you may not have fully grasped.

First, your import of java.util.Collections is superfluous and isn't used anywhere.

Next, your bound on List<Number> seems a bit...overly generic. Since these lists support Number, they could support BigDecimal and BigInteger, or more conventionally, Float and Double - which poses problems later on.

There's no reason to declare this:

Arrays.asList(new List[]{list1, list2});

Arrays.asList takes a variadic argument, so you can simply list the arguments after it without the need for an explicit array.

Arrays.asList(list1, list2);

You should look to do this for your other lists as well.

The biggest issue as far as a broken implementation that I can see is this:

if (number == num)

This is wrong for a few reasons:

  • You're using == on objects.
  • The objects you're performing this == on are not guaranteed to unbox.
  • Strict equivalence in the face of floating-point values is not entirely guaranteed.

If you wanted to fix this, you'd want to fix the bound on what your method accepts to incorporate Comparable...

private static <T extends Number & Comparable<T>> void findCountOfNumber(final List<List<T>> lists, final T num)

...and simply compare the values together.

for (T number : list) {
    if (number.compareTo(num) == 0) {
        count++;
    }
}

You'll also need to ensure that the types you use for your array and the types that you use for num are homogeneous, since it really wouldn't make sense to try to find a floating-point value in a list of integers. This is easily accomplished with the new type bounds by simply changing your lists to List<Integer>.

Lastly, let's talk about the output. This is incredibly cryptic:

List have-1
List have-5

What you're saying is, the first list has one occurrence of the number 6, and the second list has five occurrences of the number 6. But, what you're searching for and which list you're talking about aren't clear.

You should look to express that concisely. Perhaps instead of using an enhanced-for you should look to use a conventional for, which states clearly what list has what occurrences of what number.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your suggestions. I have ignored generics, output formatting and declaration of list because I was just focusing on best possible solution to find common number with it's count. \$\endgroup\$ Dec 14 '16 at 7:32
  • \$\begingroup\$ @AnkitKatiyar: The problem was that by ignoring generics, you had broken code. With generics, it's serviceable. \$\endgroup\$
    – Makoto
    Dec 14 '16 at 7:35
4
\$\begingroup\$
  • Collection is not used.
  • list1 and list2 could be private and have a better name
  • use Arrays.<Number>asList for an ArrayList<Number>. There is no need to create an Array here.
  • maybe you should flip you method parameter so support varargs
  • don't compare Objects with ==
  • the scope of your count is to large. Move it to the loop.
  • assuming that contains is also only a loop and no optimization is happening, you could just count and output the text in relation to the value. (no longer valid after changes question)
  • As Makoto stated, using Number is making you life harder then necessary. Is this really required?

With the fixes: (no longer matching the initial code, see comments)

import java.util.Arrays;
import java.util.List;
import java.util.Objects;

public class CommonNumberFinder
{

    private final static List<Number> list1 = Arrays.<Number>asList( 1, 2, 3, 4, 5, 6, 7, 8, 9 );
    private final static List<Number> list2 = Arrays.<Number>asList( 3, 5, 1, 6, 2, 6, 3, 3, 6, 1, 5, 7, 2, 6, 2, 5, 2, 6, 8, 2 );

    public static void main( String... args )
    {
        findCountOfNumber( 6, list1, list2 );
    }

    private static void findCountOfNumber( final Number num, final List<Number>... lists )
    {
        for( List<Number> list : lists )
        {
            int count = 0;
            for( Number number : list )
            {
                if( Objects.equals( number, num ) ) count++;
            }
            if( count == 0 ) System.err.printf( "Number %d is not common", num );
            else System.out.println( "List have-" + count );
        }
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ contains is a loop but not traversing the complete list. considering your solution, if I have 100 list and last list (100th one) is not having the common number, It will traverse all 99 lists completely. \$\endgroup\$ Dec 14 '16 at 7:40
  • \$\begingroup\$ But you are asking for !contains, so you have to check all elements anyway. \$\endgroup\$ Dec 14 '16 at 7:42
  • \$\begingroup\$ I guess Contains will check till it founds the element in list, not after that. So complete list will not be traversed. \$\endgroup\$ Dec 14 '16 at 7:43
  • \$\begingroup\$ Is there a return missing in your code? Do you want to abort if not all lists contain the number? Right now you are iterating throw the list regardless of the contains in the first loop. \$\endgroup\$ Dec 14 '16 at 7:46
  • \$\begingroup\$ Could use an exception for "not common", return a List<Integer> and call it in a method that handles the output. \$\endgroup\$
    – JollyJoker
    Dec 14 '16 at 14:52

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.