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

Determine if a word can be constructed from list of subsets - follow-up

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

Problem

I have reworked my code as suggested from my previous question: Determine if a word can be constructed from list of subsets.

Please instruct me on the complexity along with a review feedback, as explained in the comments below.

```
/**
* Question is better explained by examples:
* 1. given
* i/p subsets - ["un", "xy", "te", "i", "d"]
* and i/p string is : united should result in true.
* and i/p string of : union should result in false.
*
* 2. i/p subsets - ["tonyl", "pqr", "pqrbri",]
* and i/p string of : briton should be true.
* and i/p string of : japan should result in false.
*
*
* Complexity:
O(n m)
* - where n is number of characters in target
* - where m is number of subsets
*
*/
public final class StringFromSubSets {

private StringFromSubSets() {}

/**
* If a word ends with start of a target word, then return the common char count.
* Eg: for caterpillar and larva, the value returned should be 3 since,
* 'lar' is end of caterpillar which is common with 'lar' of larva.
*
* If there is no intersection then return 0.
*
* @param end The word, whose end chars should be considered to find intersection
* @param target The word, whose start chars should be considered to find intersection.
* @return The number of common chars at end of word end and start of word start.
*/
private static int subsetEndIntersectsWithTargetStart(String end, String target, int i) {
assert end != null;
assert target != null;

int j = 0;
while (i stringSubsets, String target) {
assert stringSubsets != null;
assert target != null;

for (String string : stringSubsets) {
for (int i = 0; i 0) return x;
}
}
}
return 0;
}

/**
* Returns the number of common characters in target and and subset which are common from start
* Eg:
* "winn

Solution

-
After pressing Ctrl+F (code format) in Eclipse the javadoc will be this:

/**
 * Question is better explained by examples: 1. given i/p subsets - ["un", "xy",
 * "te", "i", "d"] and i/p string is : united should result in true. and i/p
 * string of : union should result in false.


I think Eclipse is right here because usually javadoc is ends up in HTML documentation which ignores whitespaces. Try to format your javadoc that remains readable in HTML too or use non-javadoc comments:

/*
 * Question is better explained by examples:
 * 1. given
 *    i/p subsets  - ["un", "xy", "te", "i", "d"]


(Note the only one * in the first line. Eclipse does not reformat this one.)

-
It's confusing that the method name indicates a boolean return value but actually it's an integer.

private static final int anySubsetEndIntersectsWithTargetStart(List stringSubsets, String target) {
    ...
}


Inside the method the return variable is called x which still does not help. Name both of them to express their purpose.

-
private static int targetStartIntersectsWithSubsetStart(String target, String subset) {


The same issue is here. Actually, the javadoc is quite good:

/**
 * Returns the number of common characters in target and and subset which
 * are common from start Eg: "winner" and "wine" will result in return value
 * of 3, since, "win" is the intersection from start.


I'd call the method getCommonStartCharacterCount or getCommonPrefixLength. (The latter is better.) There is a similar method in Apache Commons Lang StringUtils:

String getCommonPrefix(String... strs)


One thing which does not trun out from the javadoc why the third one returns 4 here:

assertEquals(3, targetStartIntersectsWithSubsetStart("asdx", "asd")); // OK
assertEquals(3, targetStartIntersectsWithSubsetStart("asd", "asdx")); // OK
assertEquals(4, targetStartIntersectsWithSubsetStart("asdx", "asdY")); // OK


Is it intentional? Why?

(See also: Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

-
for (String string: stringSubsets) {
    for (int i = 0; i  0)
                return x;
        }
    }
}


I'd extract out a local variable here for better readability:

final char targetFirstChar = target.charAt(0);


-
while (i < target.length() && i < subset.length()) {


could be

while (i < Math.min(target.length(), subset.length())) {


I think it's easier to read since it reveals the intent.

-
You could check first that the the subsets contains every required character for the target. If not you can fail early without too much effort.

-
I'd go with something like that @rolfl suggested in this former answer. Using a higher level Set structures would improve readability and maintainability a lot.

Code Snippets

/**
 * Question is better explained by examples: 1. given i/p subsets - ["un", "xy",
 * "te", "i", "d"] and i/p string is : united should result in true. and i/p
 * string of : union should result in false.
/*
 * Question is better explained by examples:
 * 1. given
 *    i/p subsets  - ["un", "xy", "te", "i", "d"]
private static final int anySubsetEndIntersectsWithTargetStart(List<String> stringSubsets, String target) {
    ...
}
private static int targetStartIntersectsWithSubsetStart(String target, String subset) {
/**
 * Returns the number of common characters in target and and subset which
 * are common from start Eg: "winner" and "wine" will result in return value
 * of 3, since, "win" is the intersection from start.

Context

StackExchange Code Review Q#37930, answer score: 5

Revisions (0)

No revisions yet.