1
\$\begingroup\$

this code works fine but i want to know if this can be improved in any manner .

For every user I am fetching the namesList corresponding to that user and for each fetched nameList , i am fetching the ItemsList .

I have tried to replicate the sample using this below standalone program as i cant paste my project code as it got huge dependencies

So i am hardcoding the users and the names List corresponding to that user .

package com;

import java.util.ArrayList;
import java.util.List;

public class Test {

    public static void main(String args[]) {
        List<String> users = new ArrayList<String>();
        users.add("user101");
        users.add("adminuser");

        for (String user : users) {
            try {
                ArrayList<String> namesCorrespondingtoUser = getNamesCorrespondingtoUser(user);
                for (String Id : namesCorrespondingtoUser) {

                    List<String> Items = getItemsCorrespondingtoId(user, Id);

                    filter(Items, user,Id);
                }
            } catch (Exception e) {
            }
        }
    }

    public static ArrayList<String> getNamesCorrespondingtoUser(String userName)
            throws Exception {
        ArrayList<String> names = new ArrayList<String>();
        if (userName.equals("user101")) {
            names.add("UBSC1");
            names.add("HDBG1");
            names.add("GHYU1");
        }
        if (userName.equals("adminuser")) {
            names.add("UBSC1");
            names.add("HDBG1");
            names.add("GHYU1");
        }
        return names;

    }

    public static List<String> getItemsCorrespondingtoId(String userName,
            String Id) throws Exception {
            ArrayList<String> items = new ArrayList<String>();
        return items ;
    }

    private static void filter(List<String> Items, String user,String Id) {

// do something with the ID value of that User .

    }

}
\$\endgroup\$
  • \$\begingroup\$ Names is orthogonal to Items, right? That is, names are related to a user, and items are related to a user, but (user) names aren't really related to items. getItemsCorrespondingToId() is operating by user - doing it per name is just burning cycles, get the same answer multiple times. Pull the item list retrieval out of that second loop. Do you actually need to do something per-name? \$\endgroup\$ – Clockwork-Muse Jul 9 '13 at 18:37
3
\$\begingroup\$

I've seen, that you made up this code as an example, so I hope, that this is not your production code:

public static ArrayList<String> getNamesCorrespondingtoUser(String userName)
        throws Exception {
    ArrayList<String> names = new ArrayList<String>();
    if (userName.equals("user101")) {
        names.add("UBSC1");
        names.add("HDBG1");
        names.add("GHYU1");
    }
    if (userName.equals("adminuser")) {
        names.add("UBSC1");
        names.add("HDBG1");
        names.add("GHYU1");
    }
    return names;

}

If you take a look at both blocks, they are the same, aren't they? But for further improvement, I would suggest working with a Map where you have usernames as the key and the according List of Names as the result.

Second: I would reccomend you to take a look at Google's Guava Library esp. the section about predicates .

Third: I am wondering, how you could retrieve any correct item. As far as I can see your names have to be unique to resolve the correct items. The collision rate for (nick-)names is generally high. So it would be better to switch to some ID-mechanism.

Fourth: The task is in itself nested; you need to iterate over a list of items and over a list of users. So you couldn't "avoid" a nested loop. Okay: If you let a database do the magic, you didn't loop at all - the database does. But I think, that is not the answer you wanted.

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

Before avoiding to nesting for loop you must have to clean you code ..Here is the same code mentioned in your question with little bit of correction which helps you to write clean code. Changes :

  • Use interfaces instead of implementations
  • Use final in methos parameters so that reference can't be changed
  • use if else if instead of multiple if's where only 1 can be true at a time

getNamesCorrespondingtoUser

public static List<String> getNamesCorrespondingtoUser(final String userName) {
    List<String> names = new ArrayList<String>();
    if (userName.equals("user101")) {
        names.add("UBSC1");
        names.add("HDBG1");
        names.add("GHYU1");
    } else if (userName.equals("adminuser")) {
        names.add("UBSC1");
        names.add("HDBG1");
        names.add("GHYU1");
    }
    return names;
}

getItemCorrespondingToId

public static List<String> getItemsCorrespondingtoId(final String userName, final String Id) {
    List<String> items = new ArrayList<String>();
    return items ;
}

main

public static void main(String args[]) {
    List<String> users = new ArrayList<String>();
    users.add("user101");
    users.add("adminuser");

    for (String user : users) {
        try {
            List<String> namesCorrespondingtoUser = getNamesCorrespondingtoUser(user);
            for (String Id : namesCorrespondingtoUser) {
                List<String> Items = getItemsCorrespondingtoId(user, Id);
                filter(Items, user,Id);
            }
        } catch (Exception e) {
        }
    }
}
\$\endgroup\$
0
\$\begingroup\$

The code needs \$m \times n\$ loops. I don't think looping can be avoided (application or database), but a number of round-trips can be avoided if source of names and items are same. This will improve the latency introduced by repeated network calls to distant database or web-services. For example:

  1. In case of database - stored procedure can be called to return user data-set required for filter method.
  2. In case of web-service - coarse grained API calls can be made (if available from service provider).

Another way to improve the turnaround time for processing each user by introducing some parallelism; e.g., with the help of Java8's parallel streams.

users.parallelStream().forEach(user -> processUser(user));

Code inside outer loop moved into processUser() which uses parallelStream further down the line.

private static void processUser(String user) {
    try {
        getNamesCorrespondingtoUser(user).parallelStream().forEach(id -> {
            List<String> items = getItemsCorrespondingtoId(user, id);
            filter(items, user, id);
        });
    } catch(Exception e) {
    }
}
\$\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.