Is it best practices to never alter the contents of a Collection that is passed as a parameter?

I want orgSet to only have words. But, sometimes there are one char strings (like question marks, commas, etc.) that I want to remove from the set of words. These are not the full methods, but it makes my point:

 public static Set<String> removeSyntax_Foo(Set<String> orgSet) {
        Set<String> tmpSet = new HashSet();  
        for(String s : orgSet) {
            if(s.length()!=1) { tmpSet.add(s); }
            if(Character.UnicodeBlock.of(s.charAt(0)) == Character.UnicodeBlock.GENERAL_PUNCTUATION) { continue; }
        return tmpSet;

    public static void removeSyntax_Bar(Set<String> orgSet) {
        Set<String> tmpSet = new HashSet();

        for(String s : tmpSet) {
            if(s.length()!=1) { orgSet.add(s); }
            if(Character.UnicodeBlock.of(s.charAt(0)) == Character.UnicodeBlock.GENERAL_PUNCTUATION) { continue; }   

removeSyntax_Foo() has the potential for a careless memory leak. I actually have a list of static methods I use to clean-up my Set of words. If each of those methods creates a new collection... But, I feel this is the correct solution to use.
removeSyntax_Bar() jumbles the contents of the invoker's Set which might be a huge surprise to the invoker. You are not suppose to do that, right?

Is removeSyntax_Foo() or removeSyntax_Bar() best practices?

  • I disagree that this is primarily opinion-based: there are well-established and objective criteria for answering this question. – user22815 Dec 31 '15 at 17:58

The _Foo() variant, treating the input as read-only, is more thread-safe. For example, a parallel thread could do something else with origSet (again, in a read-only manner) while your code is trimming punctuation. The _Foo() variant will cause fewer surprises and probably less bugs. And, it is more flexible for future requirements. For example, somebody adds a requirement to create a set of the punctuation. Possible with the _Bar() version, but much easier with_Foo()


The answer is mostly six of one, half a dozen of the other.
For my simple need of cleansing html mark-up, numbers, syntax, etc. out of a tokenized doc, removeSyntax_bar() makes sense; those are simple utility methods. But, for interface design intended for public consumption, removeSyntax_foo() is what you're suppose to do.

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.