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

Find common "characters" in 2 given strings (rev1)

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

Problem

I would like a review of:

  • Correctness, especially around edge/corner cases



  • Efficiency



  • Readability and/or style



  • Appropriate use of Java idioms and data structures



In particular:

  • Have I missed typical idioms/patterns useful to solving the problem?



  • Could I have reused existing/popular framework/library code?



Less interesting to me:

  • Feedback on the test harness logic in main() (I could have used JUnit)



A glance at program output from online execution

I have rewritten the code below, including feedback, as Rev2 @ Find common "characters" in 2 given strings (rev2), follow the link to look at the updated version

``
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
class CommonCharacters {
@SuppressWarnings("boxing")
private static String commonCharactersOf(String string1, String string2) {
// Requirement
//
// Always return lowercase versions of common characters. e.g.:
//
// OK: (a, a) -> a
// OK: (a, A) -> a
// OK: (A, A) -> a
// No: (A, A) -> A
// No: (aA, aA) -> aA
//
// Requirement
//
// Return common characters joined in a String, preserving the order in
// which they appeared in the longest argument, or in the first argument if
// the arguments are of the same length.
//
// Requirement
//
// Handle "characters" (i.e. code points) outside the Basic Multilingual
// Plane (BMP), including characters from Supplementary Planes.
// There should be no
char' or `Character' based "false positives". i.e.:
//
// String string1 = "\uD835\uDC00", string2 = "\uD835\uDC01";
//
// string1 and string2 share no characters in the intended acceptation of
// "character".
String shortestArgument, longestArgument;
if (string1.length() codePointsSeen =
shortestArgument.codePoints()
.bo

Solution

I like that you use Streams. But I think you should use even more of their functionality:

-
You could have used map to get the lower case for longestArgument. Or maybe it would have been simpler to just use longestArgument.toLowerCase(), and same for shortestArgument.

-
You use distinct() to remove duplicate, but you could have gotten rid of the main for-loop entirely by also using filter(...) where the filter uses a predicate built from the values in shortestArgument.

-
For the final output, you can use codePointStream.toArray() to create an int[] and then use String(codePointArray, 0, codePointArray.length). You could instead use intStream.collect(...) to perform a reduction which would fill and return a StringBuilder, but it would end up quite ugly.

You spend many lines defining the shortest/longest strings, but I don't see how that is relevant. You could just assign any string to any of those and the algorithm would still work.

I might have written the program a bit differently. I would define a function that takes a string and returns the lower case version with no duplicate, in the same order, as an IntStream. I would apply that function at the start on both input strings. And then apply filter on one of them, with the predicate built from the values of the other.

Context

StackExchange Code Review Q#106473, answer score: 4

Revisions (0)

No revisions yet.