CODEWITHSUNDEEP
javamethods
Ek User
Ek User

Reputation: 53

Java best practice for calling method with return type

How can I avoid calling the same method for three times, what's the best practice in this kind of situation? Or is it the right way?

I'm checking the list is not empty first and I'm checking it again to make sure is not empty because do not wants to print empty list.

public class RegExpTest_1 {
    public static void main(String[] args) {
        ArrayList<String> list = new ArrayList<String>();
        list.add("item1");
        list.add("#item2");
        list.add("item3");
        list.add("&item4");
        list.add("item5");

        ArrayList<String> list1 = new ArrayList<String>();
        list1.add("item1");
        list1.add("item2");
        list1.add("item3");
        list1.add("item4");
        list1.add("item5");

        if (StringUtils.isNotEmpty(finditem(list)) || StringUtils.isNotEmpty(finditem(list1))){  // calling method
            if (StringUtils.isNotEmpty(finditem(list))) {         //calling same method
                System.out.println("List :\n" + finditem(list));  //calling same method
            }
            if (StringUtils.isNotEmpty(finditem(list1))) {
                System.out.println("List :\n" + finditem(list1));
            }
        }
    } //main

    public static String finditem(ArrayList<String> alist){
        StringBuilder sb = new StringBuilder();
        String re1=".*?";   // Non-greedy match on filler
        String re2="(^#|^&)";   // Word 1
        Pattern p = Pattern.compile(re1+re2,Pattern.CASE_INSENSITIVE | Pattern.DOTALL);

        for (String str:alist) {
            Matcher m = p.matcher(str);
            if (m.find()) {
              //  System.out.println("found" + m.group(1));
                sb.append(str);
                sb.append("\n");
            } else {
               // System.out.print("not found");
            }
        }

        return sb.toString();
    }
}

Upvotes: 1

Views: 109

Answers (4)

南山竹
南山竹

Reputation: 524

Your target is to print out non-empty list content, so the more better is to change your finditem method and adjust your calling methods for better structural and performance.

public class RegExpTest_1 {

public static void main(String[] args) {


    ArrayList<String> list = new ArrayList<String>();
    list.add("item1");
    list.add("#item2");
    list.add("item3");
    list.add("&item4");
    list.add("item5");

    ArrayList<String> list1 = new ArrayList<String>();
    list1.add("item1");
    list1.add("item2");
    list1.add("item3");
    list1.add("item4");
    list1.add("item5");

    /*
    if (StringUtils.isNotEmpty(finditem(list)) || StringUtils.isNotEmpty(finditem(list1))) {  // calling method
        if (StringUtils.isNotEmpty(finditem(list))) {         //calling same method
            System.out.println("List :\n" + finditem(list));  //calling same method

        }
        if (StringUtils.isNotEmpty(finditem(list1))) {
            System.out.println("List :\n" + finditem(list1));
        }
    }
    */

    final String s = finditem(list);
    if (StringUtils.isNotEmpty(s)) {
        System.out.println("List :\n" + s);
    }

    final String s1 = finditem(list1);
    if (StringUtils.isNotEmpty(s1)) {
        System.out.println("List :\n" + s1);
    }


} //main

public static String finditem(ArrayList<String> alist) {
    // Returns empty string directly if alist is null or empty
    if (null == alist || 0 == alist.size()) {
        return "";
    }

    StringBuilder sb = new StringBuilder();
    String re1 = ".*?";   // Non-greedy match on filler
    String re2 = "(^#|^&)";   // Word 1
    Pattern p = Pattern.compile(re1 + re2, Pattern.CASE_INSENSITIVE | Pattern.DOTALL);

    for (String str : alist) {

        Matcher m = p.matcher(str);
        if (m.find()) {
            //  System.out.println("found" + m.group(1));
            sb.append(str);
            sb.append("\n");
        } else {
            // System.out.print("not found");
        }
    }

    return sb.toString();
}

final static class StringUtils {
    // Do your StringUtils.isNotEmpty's do :)
    public static boolean isNotEmpty(final String s) {
        return (s != null && s.length() > 0);
    }
}
}

Upvotes: 0

Yassin Hajaj
Yassin Hajaj

Reputation: 21975

If you're using Java 8, it's a really easy implementation. With the advantage that you may modify this and add as much lists as you want just by adding them within Stream.of

  1. Make a stream of out the lists you want to finditems within
  2. Map the lists to a String by giving them as parameter to finditem
  3. Check if the String is empty, if it is, it doesn't continue the Stream pipeline
  4. Print to the console only if the returned string is not empty of course.
Stream.of(list, list1)
      .map(RegExpTest_1::finditem)
      .filter(x -> !x.isEmpty())
      .forEach(items -> System.out.println("List : \n" + items));

Upvotes: 0

Limmen
Limmen

Reputation: 1457

The outer if-statement is redundant. Just use the two inner if-statements.

e.g 

    if (StringUtils.isNotEmpty(finditem(list))) {         //calling same method
        System.out.println("List :\n" + finditem(list));  //calling same method

    }
    if (StringUtils.isNotEmpty(finditem(list1))) {
        System.out.println("List :\n" + finditem(list1));
    }

Upvotes: 2

Sergey Frolov
Sergey Frolov

Reputation: 1377

You can just use this construction:

if (StringUtils.isNotEmpty(finditem(list))) { 
    System.out.println("List :\n" + finditem(list)); 

}
if (StringUtils.isNotEmpty(finditem(list1))) {
    System.out.println("List :\n" + finditem(list1));
}

I think that external 'if' is redundant.

Upvotes: 0

Related Questions