HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Merging key-value sets as specified by a command

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
keymergingvaluecommandsetsspecified

Problem

private void merge(ArrayList operations, ArrayList> setOfStrings) {
        String toMerge = operations.get(1);
        String fromMerge = operations.get(2);
        boolean enteredFirstToMerge = false;
        boolean enteredFirstForMerge = false;
        // references that points one on toMerge and the other on fromMerge
        LinkedHashSet toMergeAndAfterDelete = null;
        LinkedHashSet addOnFromMerge = null;
        for (LinkedHashSet subSet : setOfStrings) {
        if (subSet.contains(toMerge) && subSet.contains(fromMerge))
            break;
        else {
            if (subSet.contains(toMerge) && !enteredFirstToMerge) {
            toMergeAndAfterDelete = subSet;
            enteredFirstToMerge = true;
            }
            if (subSet.contains(fromMerge) && !enteredFirstForMerge) {
            addOnFromMerge = subSet;
            enteredFirstForMerge = true;
            }
            if ((enteredFirstForMerge && enteredFirstToMerge)) {
            break;
            }
          }
        }
        /***********************************************/
        //outside Loop i call the remove on the arraylist
        //that are very expensive    
        /*************************************************/   
        if (enteredFirstForMerge && enteredFirstToMerge) {
        // first i delete from the array the linkedHashSet toMerge and
        // fromMerge and after i add a 
        // new linkedHashSet with the subSets merged
        setOfStrings.remove(toMergeAndAfterDelete);  
        setOfStrings.remove(addToMergeOnFromMerge); 
        addOnFromMerge.addAll(toMergeAndAfterDelete);
        setOfStrings.add(addOnFromMerge);
        }
}


This function takes as parameter an arrayList of operations, and an ArrayList>:

For the arrayList of operations, I always get a specific position, so I don't iterate. It is always \$O(1)\$.

For example, if I have as operation move foo bar, I have to do these steps:

-
First of all, I have to fi

Solution

-
Abstract as much as possible over type: List instead of ArrayList.

-
Use OO where appropriate: List operations should clearly be some
class MoveOperation(String to, String from).

-
It's not clear why you are using LinkedHashSet instead of List.

-
The algorithm is ill-defined: what happens if there are numerous
foo's or bar's.

I implemented something below, but it is in functional style, so it might be hard to read. There are three implicit loops: two for the filters and one for the map. You might also notice that I never modify a collection, but always create a new one. Again, this is the functional style. In your algorithm, you can actually shoot yourself in the foot if you modify some collections at the wrong moment.

private List> merge2(List operations, List> listOfSentences) {
    Stream> sentencesWithTo = listOfSentences.stream()
                                           .filter(sentence -> sentence.contains(toMerge));
    Stream> sentencesWithFrom = listOfSentences.stream()
                                             .filter(sentence -> sentence.contains(fromMerge));

    long nTo = sentencesWithTo.count();
    long nFrom = sentencesWithFrom.count();

    if (nTo == 0  || nFrom == 0) {
        return listOfSentences;
    } else if (nTo > 1 || nFrom > 1) {
        // what to do here??
    } else {
        List sentenceTo = sentencesWithTo.collect(Collectors.toList()).get(0); // should only be one element ?
        List sentenceFrom = sentencesWithFrom.collect(Collectors.toList()).get(0); // should only be one element ?
        Stream> updatedSentences = listOfSentences.stream().map(sentence -> {
            if (sentence.equals(sentenceTo))
                return Stream.concat(sentenceTo.stream(), sentenceFrom.stream()).collect(Collectors.toList());
            else if (sentence.equals(sentenceFrom))
                return new ArrayList<>();
            else 
                return sentence;
        });
        return updatedSentences.collect(Collectors.toList());
    }
}

Code Snippets

private List<List<String>> merge2(List<String> operations, List<List<String>> listOfSentences) {
    Stream<List<String>> sentencesWithTo = listOfSentences.stream()
                                           .filter(sentence -> sentence.contains(toMerge));
    Stream<List<String>> sentencesWithFrom = listOfSentences.stream()
                                             .filter(sentence -> sentence.contains(fromMerge));

    long nTo = sentencesWithTo.count();
    long nFrom = sentencesWithFrom.count();

    if (nTo == 0  || nFrom == 0) {
        return listOfSentences;
    } else if (nTo > 1 || nFrom > 1) {
        // what to do here??
    } else {
        List<String> sentenceTo = sentencesWithTo.collect(Collectors.toList()).get(0); // should only be one element ?
        List<String> sentenceFrom = sentencesWithFrom.collect(Collectors.toList()).get(0); // should only be one element ?
        Stream<List<String>> updatedSentences = listOfSentences.stream().map(sentence -> {
            if (sentence.equals(sentenceTo))
                return Stream.concat(sentenceTo.stream(), sentenceFrom.stream()).collect(Collectors.toList());
            else if (sentence.equals(sentenceFrom))
                return new ArrayList<>();
            else 
                return sentence;
        });
        return updatedSentences.collect(Collectors.toList());
    }
}

Context

StackExchange Code Review Q#53899, answer score: 3

Revisions (0)

No revisions yet.