1
\$\begingroup\$

I know that I'm not suppose to alter a Set as I iterate through it. But, in the deleteRow(String counter, String reading) method below, doing just that will make the code much more readable and clean.

Because there is so little going on in each iteration:

  1. Is there risk?
  2. Would a respectable coder look at that and consider it flawed?

public static class DbTable {
    public enum Columns { counter, reading, dfEng }
    public Set<Row> tableData = new HashSet<Row>();

    public void deleteRow(String counter, String reading) {
        Row deleteThis = null;
        for(Row r : tableData) {
            if((r.counter.equals(counter)) && (r.reading.equals(reading))) {
                // tableData.remove(r); <--- I want to do this, and 
                // return; <--- then this to have cleaner code.
                deleteThis = r;
                break;
            }
        }
        if(deleteThis != null) { tableData.remove(deleteThis); }
    }
            public static class Row {
                public final String counter, reading, dfEng;
                public Row(String counter, String reading, String dfEng) {
                    this.counter = counter;
                    this.reading = reading;
                    this.dfEng = dfEng;
                }

                public String toString() {
                    String s = counter + String.valueOf((char) 9) + reading + String.valueOf((char) 9) + dfEng;
                    return s;
                }
            }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ What you possibly need is an Iterator, and its remove() method. \$\endgroup\$ – h.j.k. May 5 '15 at 17:26
  • \$\begingroup\$ Recommended reading: Removing elements on a List while iterating through it (most of it can be applied on a Set too), I think you might find your answer there. \$\endgroup\$ – Simon Forsberg May 5 '15 at 17:44
  • \$\begingroup\$ @SimonAndréForsberg So, I conclude that the appropriate solution for me is Interator.remove(); . But, as I kick-out of the iteration immediately after the Set alteration, I'm still not sure if there really is risk. Even so, I'm using Interator.remove(). thanks. \$\endgroup\$ – red shoe May 5 '15 at 19:41
4
\$\begingroup\$

No, there is no risk because you are returning directly after removing, or breaking directly after setting the deleteThis variable.

However, to make each part of your code as safe as possible, I would recommend to use the Iterator approach. The idea is that if you would forget to use break; or return; then you would have problems. If you use Iterator.remove(), then you are still safe.

There is another variant as well, based on Java 8 (if you have that available),

tableData.stream().filter(r -> r.counter.equals(counter)
        && r.reading.equals(reading))
    .findAny()
    .ifPresent(r -> tableData.remove(r));

This will not have a big impact on performance thanks to the fact that you are using a Set.

Now, I am not fully aware of your exact situation but another option might be to implement hashCode and equals in your Row class, in which case two Row objects with the same properties won't be added to the Set in the first place, and if you want to remove one and replace with another, you can do so with \$O(1)\$ complexity instead of your current \$O(n)\$. This assumes though that you are actually free to do this. Implementing hashCode and equals can have other unforeseen consequences.

Other comments

  • enum values should be named UPPERCASE according to Java naming conventions.
  • It is unclear what a Row really represents, and what a dfEng is.
  • Since Java 7, you can use the "diamond operator" <>, you can do new HashSet<>();
  • Not necessary to store the String in a local variable in toString (although it can be useful if you want to debug and see what the method will return).
\$\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.