In this class I store a number and content belonging to this number. In the method deleteContentIfOlderThen() I check if the ArrayList(opexContent) is empty. But this works only if the check occurs inside the for loop. If I check it outside it doesn't work. I use a workStealingExecutorService to execute this method. But the list is only accessed by this class while the threads do the work.
So what am I doing wrong that it doesn't work outside the loop?
new version with iterator:
package org.excelAnalyser;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.Iterator;
public class OpexLine implements Runnable{
public static final int DELETE_CONTENT_TASK=0;
public final int SAPNumber;
public final ArrayList<String[]> opexContent;
private final DeletionRequestHandler m_deletionRequestHandler;
private String[] m_taskParameters=null;
private int m_taskid=-1;
public OpexLine(int SAPNumber, ArrayList<String[]> opexContent, DeletionRequestHandler deletionRequestHandler) {
this.SAPNumber=SAPNumber;
this.opexContent=opexContent;
m_deletionRequestHandler=deletionRequestHandler;
}
public void setupTask(int taskID, String[] parameters) {
m_taskid=taskID;
m_taskParameters=parameters;
}
public void run() {
if(m_taskid<0) {
System.out.println("aborted task because no taskID was given");
return;
}
if(m_taskParameters==null) {
System.out.println("no taskparameters were specified");
return;
}
switch (m_taskid) {
case DELETE_CONTENT_TASK:
deleteContentIfOlderThen();
break;
}
}
private void deleteContentIfOlderThen() {
final long unixTime=Long.parseLong(m_taskParameters[0]);
final int indexOfTime=Integer.parseInt(m_taskParameters[1]);
final SimpleDateFormat sdf = new SimpleDateFormat("dd.MM.yyyy");
Iterator<String[]> opexContentIterator=opexContent.iterator();
contentLoop: while(opexContentIterator.hasNext()) {
String[] content=(String[])opexContentIterator.next();
final String timeString=content[indexOfTime];
if(timeString==null || timeString.isEmpty()) {
continue contentLoop;
}
Date date = null;
try {
date = sdf.parse(timeString);
} catch (Exception e) {
continue contentLoop;
//date = new Date();
}
Date checkDate=new Date(System.currentTimeMillis()-unixTime);
if(date.before(checkDate)) {
opexContentIterator.remove();
}
}
if(opexContent.isEmpty()) {
m_deletionRequestHandler.requestDeletionFor(this);
}
}
public String toString() {
final StringBuilder stringBuilder=new StringBuilder();
stringBuilder.append(SAPNumber);
stringBuilder.append(": ");
final int length=String.valueOf(SAPNumber).length();
//gen offset
final StringBuilder offsetBuilder=new StringBuilder();
for(int i=0; i<length 2; i ) {
offsetBuilder.append(' ');
}
final String offset=offsetBuilder.toString();
for (String[] strings : opexContent) {
for (String string : strings) {
stringBuilder.append(string "; ");
}
stringBuilder.append("\n" offset);
}
return stringBuilder.toString();
}
}
old version without iterator:
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
public class OpexLine implements Runnable{
public static final int DELETE_CONTENT_TASK=0;
public final int SAPNumber;
public final ArrayList<String[]> opexContent;
private final DeletionRequestHandler m_deletionRequestHandler;
private String[] m_taskParameters=null;
private int m_taskid=-1;
public OpexLine(int SAPNumber, ArrayList<String[]> opexContent, DeletionRequestHandler deletionRequestHandler) {
this.SAPNumber=SAPNumber;
this.opexContent=opexContent;
m_deletionRequestHandler=deletionRequestHandler;
}
public void setupTask(int taskID, String[] parameters) {
m_taskid=taskID;
m_taskParameters=parameters;
}
public void run() {
if(m_taskid<0) {
System.out.println("aborted task because no taskID was given");
return;
}
if(m_taskParameters==null) {
System.out.println("no taskparameters were specified");
return;
}
switch (m_taskid) {
case DELETE_CONTENT_TASK:
deleteContentIfOlderThen();
break;
}
}
private void deleteContentIfOlderThen() {
final long unixTime=Long.parseLong(m_taskParameters[0]);
final int indexOfTime=Integer.parseInt(m_taskParameters[1]);
final SimpleDateFormat sdf = new SimpleDateFormat("dd.MM.yyyy");
contentLoop: for(final String[] content : opexContent) {
//final String[] content=opexContent.get(i);
final String timeString=content[indexOfTime];
if(timeString==null || timeString.isEmpty()) {
continue contentLoop;
}
Date date = null;
try {
date = sdf.parse(timeString);
} catch (Exception e) {
continue contentLoop;
//date = new Date();
}
Date checkDate=new Date(System.currentTimeMillis()-unixTime);
//System.out.println(date " before " checkDate ": " date.before(checkDate));
if(date.before(checkDate)) {
System.out.println("removed");
opexContent.remove(content);
if(opexContent.isEmpty()) {
System.out.println("isEmptyAfterRemoval");
m_deletionRequestHandler.requestDeletionFor(this);
}
}
}
//System.out.println("isexecuted");
if(opexContent.isEmpty()) {
System.out.println("isEmpty");
m_deletionRequestHandler.requestDeletionFor(this);
}
}
public String toString() {
final StringBuilder stringBuilder=new StringBuilder();
stringBuilder.append(SAPNumber);
stringBuilder.append(": ");
final int length=String.valueOf(SAPNumber).length();
//gen offset
final StringBuilder offsetBuilder=new StringBuilder();
for(int i=0; i<length 2; i ) {
offsetBuilder.append(' ');
}
final String offset=offsetBuilder.toString();
for (String[] strings : opexContent) {
for (String string : strings) {
stringBuilder.append(string "; ");
}
stringBuilder.append("\n" offset);
}
return stringBuilder.toString();
}
}
CodePudding user response：
You should never mutate a list while you are iterating it. This type of code:
final ArrayList<String[]> opexContent;
private void deleteContentIfOlderThen() {
for(final String[] content : opexContent) {
Date date = /* some date */;
Date checkDate=new Date(System.currentTimeMillis()-unixTime);
if(date.before(checkDate)) {
opexContent.remove(content); //CME!
}
}
}
Will likely cause a ConcurrentModificationException, as the list was mutated while an Iterator was in use (behind the scenes). Instead, you should use the Iterator directly, such that you have the ability to use Iterator#remove:
final ArrayList<String[]> opexContent;
private void deleteContentIfOlderThen() {
Iterator<String[]> itr = opexContent.iterator();
while (itr.hasNext()) {
String[] content = itr.next();
Date date = /* some date */;
Date checkDate=new Date(System.currentTimeMillis()-unixTime);
if(date.before(checkDate)) {
itr.remove(); //No more CME!
}
}
}
CodePudding user response：
Whilst Rogue's answer is correct in saying that you shouldn't remove elements from the list while iterating it, there's a neater way of removing the elements since Java 8 that avoids using an iterator explicitly:
// Doesn't have to be moved outside, but at least doing this means
// you're comparing against a fixed check date, rather than potentially
// changing as you iterate.
Date checkDate=new Date(System.currentTimeMillis()-unixTime);
opexContent.removeIf(content -> {
Date date = /* some date */;
return date.before(checkDate);
});
This is better because
- it's clearer, because you're not having to deal with the iterator
- it's safer, as it leaves the list untouched if an exception is thrown for some element (i.e. it's failure atomic);
- it's more efficient, because it avoids resizing the list repeatedly