5
\$\begingroup\$

I've been working on this elevator program for quite some time now and finally finished it and made it work and I'm pretty proud of myself, but I'd like to see ways how to refactor my code so I can learn for the future. By refactoring I mean, making the code look better, maybe by using less lines of codes.

public class Test {

    public static void main( String[] args) {

        Random r = new Random();
        List<Integer> nums = new ArrayList<Integer>();
        for ( int i = 0; i < 100000; i++) {
            nums.add( r.nextInt( 100000) * ((i % 4 == 0) ? 1 : -1));
        }

        long t = System.currentTimeMillis();
        // part 1

        int lastPositivePos = getLastPositivePosition( nums);
        System.out.println( "lastPositivePos " + lastPositivePos + " has value " + nums.get( lastPositivePos));

        Set<Integer> positiveNumbers = removeNegativeNumbers( nums);
        Set<Integer> negativeNumbers = removePositiveNumbers( nums);

        System.out.println( positiveNumbers.size() + " + " + negativeNumbers.size() + " should be 5000");

        // part 2
        List<Integer> positiveList = repeatElements( positiveNumbers, 5);
        List<Integer> negativeList = repeatElements( negativeNumbers, 5);
        Collection<Integer> merged = mergeAndSort( negativeList, positiveList);
        System.out.println( merged.size() + " sorted elements");
        // part 3
Map<Integer, Integer> nums2 = new ConcurrentHashMap<Integer, Integer>();

        for ( int i = 0; i < 1000; i++) {
            nums2.put( r.nextInt( 100), r.nextInt( 100));
        }
        mergeEvenNumbers( nums2);

        long d = System.currentTimeMillis() - t;
        System.out.println( "test takes " + d);
    }

    private static List<Integer> repeatElements( Collection<Integer> nums, int repeat) {
        List<Integer> result = new ArrayList<Integer>();
        for ( int i = 0; i < repeat; i++) {
            for ( int num : nums) {
                result.add( num);
            }
        }
        return result;
    }

    /**
     * if two keys are even, add the keys together and the values together, until only one even key remains.
     * 
     * @param nums2
     */
    private static void mergeEvenNumbers( Map<Integer, Integer> nums2) {

        int currentSize = 0;
        while ( currentSize != nums2.size()) {
            currentSize = nums2.size();
            int evenKey = -1;

            for ( int key : nums2.keySet()) {
                if ( key % 2 == 1) {
                    continue;
                }
                if ( evenKey < 0) {
                    evenKey = key;
                } else {
                    // merge keys and values
                    int val1 = nums2.remove( evenKey);
                    int val2 = nums2.remove( key);
                    nums2.put( key + evenKey, val1 + val2);
                    evenKey = -1;
                }
            }

        }
    }

    private static Collection<Integer> mergeAndSort( List<Integer> nums1, List<Integer> nums2) {
        List<Integer> result = new ArrayList<Integer>();
        result.addAll( nums1);
        result.addAll( nums2);
        Collections.sort( result);
        return result;
    }

    private static Set<Integer> removePositiveNumbers( List<Integer> list) {
        for ( int i = 0; i < list.size(); i++) {
            if ( list.get( i) > 0) {
                list.remove( i);
            }
        }
        return new HashSet<Integer>( list);
    }

    private static Set<Integer> removeNegativeNumbers( List<Integer> list) {
        for ( int i = 0; i < list.size(); i++) {
            if ( list.get( i) < 0) {
                list.remove( i);
            }
        }
        return new HashSet<Integer>( list);
    }

    private static int getLastPositivePosition( List<Integer> nums) {
        int result = -1;
        for ( int i = 0; i < nums.size(); i++) {
            if ( nums.get( i) > 0) {
                result = i;
            }
        }
        return result;
    }
}
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

Your code is an odd collection of loosely related functions, many of which have solutions in the Java Streams API. In addition, you have odd signatures that don't make sense.

I look at methods like:

private static Set<Integer> removeNegativeNumbers( List<Integer> list) {
    for ( int i = 0; i < list.size(); i++) {
        if ( list.get( i) < 0) {
            list.remove( i);
        }
    }
    return new HashSet<Integer>( list);
}

And I think: Why:

  • return a Set when the input is a List
  • why remove the data from the input list, and then return it in a set (you do know that remove() on ArrayList is really slow?)
  • have an input of a list of Integer, why not an array of int? If you have Integer, will you have Long too?, etc.
  • if you return a set, why force a List as input, why not just a Collection, or Iterable?

For multiple reasons, mostly for performance, though, I would have something like:

private static <N extends Number> Set<N> removeNegativeNumbers( Iterable<N> data) {
    return StreamSupport.stream(data.spliterator())
            .filter(n -> Math.signum(n.doubleValue()) >= 0.0)
            .collect(Collectors.toSet());
}

That will return all non-negative values for any of the number types.

It iwll work for List<Integer>, List<Double>, List<BigDecimal>, Set<Float>, etc.

Using generics, streams, and appropriate collections is the first step in making reusable code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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