10
\$\begingroup\$

I want to read a list of elements and pass a maximum of 99 elements to a function for some logical operations.

I have tried this with an array as an example and this code was successful in achieving my purpose.

I just want someone to review it and help optimize it.

/** 
 */
package com.review.code.java;

/**
 * i>   Read a List Input,
 * ii>  Call a Function with maximum of 99 elements in one iteration Eg:(0..98; 99..198; 199..297; .....)
 * 
 *
 */
public class OptimizeReadingList {

    public static int list_size=107;//252,543,... etc. - Input List Size

    public static int a[] = new int[list_size];

    public static void main(String[] args) {

        int read_max_size = 99;
        int input_list_size = list_size;

        // Add Elements to the Array
        for(int j =0;j<input_list_size;j++){
            a[j]=j+1;
        }

        // Print all the Elements in the List 
        for(int j =0;j<input_list_size;j++){
            System.out.print(a[j]+",");
        }

        System.out.println("\n Print only a max of 99 elements in one iteration using the printElements method");

        // Should Call a function printElements with start_index and end_index
        for(int i = 0,ele = (read_max_size-1); i<input_list_size ; ){

            printElements(i,ele);

            // Increment Operations for the next elements(maybe another 99 or less than 99)
            i=(ele+1);
            if((ele+read_max_size > input_list_size)){
                ele=input_list_size;
            }else{
                ele=ele+ele;
            }
        }

    }

    // Consider that this method can read only a maximum of 99 elements in range.
    private static void printElements(int i, int ele) {
        for(int j = i;j<ele;j++){
            System.out.print(a[j]+",");
        }
        System.out.println("\n");
    }

}
\$\endgroup\$
  • \$\begingroup\$ Don't use '_' in variable names except for "public static final" ones. Take a look at the following site - especially the third point of the "Naming" section: docs.oracle.com/javase/tutorial/java/nutsandbolts/… \$\endgroup\$ – Datagrammar Jun 27 '15 at 11:03
  • 1
    \$\begingroup\$ Note that the backticks are for the markup of code samples, not for everything that you want to be emphasized (there is Strong and Emphasis for that purpose). \$\endgroup\$ – Martin R Jun 27 '15 at 11:06
14
\$\begingroup\$

Bugs

Your code doesn't actually do what it says it will do. The problems are here:

    for(int i = 0,ele = (read_max_size-1); i<input_list_size ; ){

        printElements(i,ele);

        // Increment Operations for the next elements(maybe another 99 or less than 99)
        i=(ele+1);
        if((ele+read_max_size > input_list_size)){
            ele=input_list_size;
        }else{
            ele=ele+ele;
        }
    }

Problem one

On the first iteration, only 98 items will be printed instead of 99. This is because ele is set to read_max_size-1 which is 98, so printElements() will end up printing the elements from 0..97 instead of 0..98.

Problem two

After each iteration, you do ele=ele+ele;. This is incorrect because it doubles the end index instead of adding 99 to it. So while the 2nd iteration will be ok, the third iteration will end up printing 198 elements instead of 99. The fourth iteration will print 396 elements, etc.

You should replace that line with ele += read_max_size;.

Problem three

On the first iteration, there is no check to see if ele exceeds the array bounds. So if the array is smaller than read_max_size, you will get an out of bounds exception.

Problem four

You are setting i=(ele+1); after each iteration. By doing that, you are skipping one element. It should be i = ele; instead.

Rewritten loop

Here is how the loop could have been written to avoid the problems:

    for (int start = 0; start < input_list_size; start += read_max_size) {
        int end = start + read_max_size;
        if (end > input_list_size) {
            end = input_list_size;
        }
        printElements(start, end);
    }
\$\endgroup\$
15
\$\begingroup\$

First things first, I feel the biggest problem about your code is that your comments lie.

/**
 * i>   Read a List Input,
 * ii>  Call a Function with maximum of 99 elements in one iteration Eg:(0..98; 99..198; 199..297; .....)
 * 
 *
 */
public class OptimizeReadingList

There is no List to be seen, and I am not sure what is meant by 'call a function'. Javadocs generally explain what the class/method is supposed to be used for, and they hardly talk about their underlying/reliant methods (except when the @see literal is used). At the very most, sometimes Javadocs cover "implementation notes" or "API notes" highlighting the caveats of such an implementation, but that's all to it.

// Should Call a function printElements with start_index and end_index
for(int i = 0,ele = (read_max_size-1); i<input_list_size ; ){

Should it, should it not? Why so? Even for general Javadoc comments, it sounds weird to be suggesting something so specific... if you decide to rename your method down the road, you'll have to update this comment, which becomes cumbersome in the long run.

// Increment Operations for the next elements(maybe another 99 or less than 99)
i=(ele+1);

Let's start printing batches of 98 instead. You'll have to manually update refrences to 99 throughout your code. Comments should document the 'why' and not the 'how', hence your comments describing how you are selecting maybe the next 99, or less than 99, is usually not recommended.

// Consider that this method can read only a maximum of 99 elements in range.
private static void printElements(int i, int ele) {

I'm only highlighting the method declaration, but nowhere in the method says that a maximum of 99 elements will be read. If I call this as printElements(0, 0);, I will only get a newline output. If I call this as printElements(9000, 10000);, I will probably get an ArrayIndexOutOfBoundsException exception (since you have hard-coded your test array as 107 elements).

So how can I actually achieve what I need?

You say you need a List, so let's see if there are any methods that can help you out with...

List<E> subList(int fromIndex, int toIndex)

Returns a view of the portion of this list between the specified fromIndex, inclusive, and toIndex, exclusive. (If fromIndex and toIndex are equal, the returned list is empty.) The returned list is backed by this list, so non-structural changes in the returned list are reflected in this list, and vice-versa. The returned list supports all of the optional list operations supported by this list.

private static final int BATCH_SIZE = 99;

private static <T> void splitProcess(List<T> list) {
    splitProcess(list, BATCH_SIZE);
}

private static <T> void splitProcess(List<T> list, int batchSize) {
    if (batchSize < 1) {
        throw new IllegalArgumentException("Batch size must not be less than 1.");
    }
    for (int start = 0, end; start < (end = Math.min(list.size(), start + batchSize));
            start = end) {
        System.out.println(list.subList(start, end));
    }
}
  1. Define your desired batch size as BATCH_SIZE = 99.
  2. Have a 1-argument splitProcess(List) method that calls the 'fuller' splitProcess(List, int) method with BATCH_SIZE as the default second argument.
  3. Do some validation for batchSize.
  4. Construct a for-loop with:
    • two variables representing the start (inclusive) and end (exclusive) points
    • looping while start < list.size(), and
    • increment them accordingly.

Then you can call list.subList(start, end), and use it in the way you require.

Javadocs shall be left as an exercise for the reader...

\$\endgroup\$
  • 1
    \$\begingroup\$ for (int start = 0, end ; start < (end = Math.min(list.size(), start + batchSize)) ; start = end) (A pity Spliterator doesn't provide for specification of "split point". Streams & lambda?) \$\endgroup\$ – greybeard Jan 10 '16 at 8:25
  • \$\begingroup\$ @greybeard thanks for the suggestion! That does improve the readability slightly, have updated the answer. :) \$\endgroup\$ – h.j.k. Jan 10 '16 at 9:12
13
\$\begingroup\$

Let's take a look at naming and style.

public class OptimizeReadingList {

That sounds like a function, not like a class.

        // Add Elements to the Array
        for(int j =0;j<input_list_size;j++){
            a[j]=j+1;
        }

Comments should explain why, not how. Also, your spacing is inconsistent. The following would be more readable:

        for(int j = 0; j < input_list_size; j++){
            a[j] = j + 1;
        }

The same goes for many of your functions, including:

        for(int i = 0,ele = (read_max_size-1); i<input_list_size ; ){

            printElements(i,ele);

            // Increment Operations for the next elements(maybe another 99 or less than 99)
            i=(ele+1);
            if((ele+read_max_size > input_list_size)){
                ele=input_list_size;
            }else{
                ele=ele+ele;
            }
        }

This could be rewritten for increased readability.

        for(int i = 0, ele = (read_max_size-1); i < input_list_size){
            printElements(i, ele);

            // Increment Operations for the next elements(maybe another 99 or less than 99)
            i = (ele + 1);
            if(ele + read_max_size > input_list_size){
                ele = input_list_size;
            } else {
                ele += ele;
            }
        }

Naming is important. Try to avoid variable names like a, and ele. i and j are widely accepted as iterators, so those are fine.

\$\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.