current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

I am trying to remove empty a String array from ArrayList:

  List<String[]> someList = new ArrayList<String[]>();

which contains data like:

[
  { "Loic" , "Remy" , "Fail","Medical"} ,
  { "Faser" , "Foster" , "Pass","Southampton","GK","Young"} ,
  { "" , "" ,} ,
  { "" , "" , "","","",""} ,
  { "Emre" , "Can" , "Pass","Liverpool","CDM"}
]

I want to remove the empty String[] from the ArrayList:

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
    //Clone to new Array List
    List<String[]> cloneCSV = new ArrayList<String[]>(someList);
    for (String[] csvSingleLine : cloneCSV) {
        //Create New List of String only
        List<String> strings = new ArrayList<String>(Arrays.asList(csvSingleLine));
        //Remove all string that are null or blank
        strings.removeAll(Arrays.asList(null, ""));
        //Check if the List is empty or null after removing in above step
        if (strings.isEmpty() || Validator.isNull(strings)) {
            //If yes it is all blank String array 
            //Remove from orginal LIST
            someList.remove(csvSingleLine);
        }
    }

The above code is working fine for me.

Questions:

  1. Is there any other alternative solution that is quick and elegant?
  2. What are the change that I could make to it, to make it more efficient?

The size of each row varies, so I couldn't use remove all by creating a dummy array.

share|improve this question
2  
I'd like to mention, that the excessive commenting actually makes this less readable. –  Cruncher 2 days ago
    
Yes, in my development environment, i don't use comments that often....except for the javadoc. But i commented it out here so , i can elaborate what i have done in the code. –  Runcorn 2 days ago
    
@Runcorn We can always see what you are doing, the code will tell us that. Your comments should explain why you are doing it. –  Simon André Forsberg 2 days ago

2 Answers 2

up vote 10 down vote accepted

There are a few tricks you can use to improve this code.

First of all,

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
List<String[]> cloneCSV = new ArrayList<String[]>(someList);
for (String[] csvSingleLine : cloneCSV) {

I understand that you're using this "copy the list"-approach to avoid a ConcurrentModificationException, but instead of doing that you can use an Iterator.

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
Iterator<String[]> iterator = someList.iterator();
while (iterator.hasNext()) {
    String[] strings = iterator.next();
    if (stringsShouldBeRemoved) {
        iterator.remove();
    }
}

Now, as for your code to check for whether to remove a String[]:

List<String> strings = new ArrayList<String>(Arrays.asList(csvSingleLine));
strings.removeAll(Arrays.asList(null, ""));
if (strings.isEmpty() || Validator.isNull(strings)) {
    someList.remove(csvSingleLine);
}

Again, you're using a copying-approach. To check for whether or not an object fulfills a criteria, you don't need to copy it, modify it, and check if it fulfills another criteria. Copying it and modifying it makes it slower and uses more memory.

Instead, let's use a method:

boolean shouldRemoveCSVSingleLine(String[] csvSingleLine) {
    for (String str : csvSingleLine) {
        if (str != null && !str.equals("")) {
            return false;
        }
    }
    return true;
}

Now you can use this method in the while(iterator.hasNext()) loop:

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
Iterator<String[]> iterator = someList.iterator();
while (iterator.hasNext()) {
    String[] strings = iterator.next();
    if (shouldRemoveCSVSingleLine(strings)) {
        iterator.remove();
    }
}
share|improve this answer
    
Thanks @Smion , i agree with you in the most of the reasoning you stated , except for checking the empty array. for (String str : csvSingleLine) { , wouldn't it be better to use csvSingleLine.removeAll(Arrays.asList(null, "")); ? –  Runcorn Aug 12 at 9:18
    
@Runcorn That depends, if you have the data { "Faser", "Foster", "Pass", "", "", "Young"} would you want it to be modified into { "Faser", "Foster", "Pass", "Young"} ? –  Simon André Forsberg Aug 12 at 9:22
    
Ah , Ok . Thanks. –  Runcorn Aug 12 at 9:26
up vote 5 down vote

The removal code can be done in 1 line using the Java 8 Stream API, though I'm sure Simon's answer has better performance.

static List<String[]> removeEmptyStringArrays(List<String[]> strArrays) {
    return strArrays
            .stream()
            .filter(strArray -> !isEmptyStringArray(strArray))
            .collect(Collectors.toList());
}

static boolean isEmptyStringArray(String[] strArray) {
    for(String str : strArray) {
        if(str != null && !str.equals("")) {
            return false;
        }
    }
    return true;
}
share|improve this answer
    
You could just write if (!"".equals(str)) { return false; }. –  200_success♦ 2 days ago
3  
Speaking of Java 8, it's even possible to do return Arrays.stream(args).allMatch(!"".equals(str)); in the isEmptyStringArray method, which makes it easier to inline it inside the .filter lambda directly. –  Simon André Forsberg 2 days ago
    
+1 @emagdne Thanks for the answer, I have not yet ported to Java 8 .. But hey i will someday .. :D –  Runcorn 2 days ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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