Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Don't add unnecessary pre-checks

Don't add unnecessary pre-checks

Your method getSubstringsSet starts with two early returns:

if(aString.length() < lengthOfSubstring){
 return setOfSubstrings;
}
if(aString.length() == lengthOfSubstring){
 setOfSubstrings.add(aString);
 return setOfSubstrings;
}

You actually don't need them. The rest of the code handles both cases just fine and it doesn't add to clarity or improve performance. Early-returns should be used when the rest of the code can then assume a condition that was checked, making its code simpler; but it is not the case here.

###Use built-in methods

Use built-in methods

To add the substrings, you are currently doing:

StringBuilder sb = new StringBuilder();
//add each substring of length (lengthOfSubstring) to the setOfSubstrings. 
for(int j = 0; j < lengthOfSubstring; j++){
 sb.append(charArray[i + j]);
}
setOfSubstrings.add(sb.toString());

So you're creating a new StringBuilder and then appending each character. This is a complicated way of retrieving the substring between two indices. It also allocates a new StringBuilder each time which isn't really needed.

You can just have

setOfSubstrings.add(aString.substring(i, i + lengthOfSubstring));

In the same way, stringIntersect is a lot more complicated than it should be.

First of all, you are using a label outerForLoop and then using it to break out of the inner loop with break outerForLoop;. This is generally not a good practice. What this shows is a missing method: what you really want is to make a method isIntersection(set1, set2) that would determine if the two sets have elements in common.

However, you don't even need to write one, since it is already built-in: Collections#disjoint. This method returns true if the two given collections are disjoint, i.e. have no elements in common. Therefore, you can remove your double for loop and just have:

boolean sameSubstringInBothStrings = !Collections.disjoint(setOne, setTwo);

###Don't add unnecessary pre-checks

Your method getSubstringsSet starts with two early returns:

if(aString.length() < lengthOfSubstring){
 return setOfSubstrings;
}
if(aString.length() == lengthOfSubstring){
 setOfSubstrings.add(aString);
 return setOfSubstrings;
}

You actually don't need them. The rest of the code handles both cases just fine and it doesn't add to clarity or improve performance. Early-returns should be used when the rest of the code can then assume a condition that was checked, making its code simpler; but it is not the case here.

###Use built-in methods

To add the substrings, you are currently doing:

StringBuilder sb = new StringBuilder();
//add each substring of length (lengthOfSubstring) to the setOfSubstrings. 
for(int j = 0; j < lengthOfSubstring; j++){
 sb.append(charArray[i + j]);
}
setOfSubstrings.add(sb.toString());

So you're creating a new StringBuilder and then appending each character. This is a complicated way of retrieving the substring between two indices. It also allocates a new StringBuilder each time which isn't really needed.

You can just have

setOfSubstrings.add(aString.substring(i, i + lengthOfSubstring));

In the same way, stringIntersect is a lot more complicated than it should be.

First of all, you are using a label outerForLoop and then using it to break out of the inner loop with break outerForLoop;. This is generally not a good practice. What this shows is a missing method: what you really want is to make a method isIntersection(set1, set2) that would determine if the two sets have elements in common.

However, you don't even need to write one, since it is already built-in: Collections#disjoint. This method returns true if the two given collections are disjoint, i.e. have no elements in common. Therefore, you can remove your double for loop and just have:

boolean sameSubstringInBothStrings = !Collections.disjoint(setOne, setTwo);

Don't add unnecessary pre-checks

Your method getSubstringsSet starts with two early returns:

if(aString.length() < lengthOfSubstring){
 return setOfSubstrings;
}
if(aString.length() == lengthOfSubstring){
 setOfSubstrings.add(aString);
 return setOfSubstrings;
}

You actually don't need them. The rest of the code handles both cases just fine and it doesn't add to clarity or improve performance. Early-returns should be used when the rest of the code can then assume a condition that was checked, making its code simpler; but it is not the case here.

Use built-in methods

To add the substrings, you are currently doing:

StringBuilder sb = new StringBuilder();
//add each substring of length (lengthOfSubstring) to the setOfSubstrings. 
for(int j = 0; j < lengthOfSubstring; j++){
 sb.append(charArray[i + j]);
}
setOfSubstrings.add(sb.toString());

So you're creating a new StringBuilder and then appending each character. This is a complicated way of retrieving the substring between two indices. It also allocates a new StringBuilder each time which isn't really needed.

You can just have

setOfSubstrings.add(aString.substring(i, i + lengthOfSubstring));

In the same way, stringIntersect is a lot more complicated than it should be.

First of all, you are using a label outerForLoop and then using it to break out of the inner loop with break outerForLoop;. This is generally not a good practice. What this shows is a missing method: what you really want is to make a method isIntersection(set1, set2) that would determine if the two sets have elements in common.

However, you don't even need to write one, since it is already built-in: Collections#disjoint. This method returns true if the two given collections are disjoint, i.e. have no elements in common. Therefore, you can remove your double for loop and just have:

boolean sameSubstringInBothStrings = !Collections.disjoint(setOne, setTwo);
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

###Don't add unnecessary pre-checks

Your method getSubstringsSet starts with two early returns:

if(aString.length() < lengthOfSubstring){
 return setOfSubstrings;
}
if(aString.length() == lengthOfSubstring){
 setOfSubstrings.add(aString);
 return setOfSubstrings;
}

You actually don't need them. The rest of the code handles both cases just fine and it doesn't add to clarity or improve performance. Early-returns should be used when the rest of the code can then assume a condition that was checked, making its code simpler; but it is not the case here.

###Use built-in methods

To add the substrings, you are currently doing:

StringBuilder sb = new StringBuilder();
//add each substring of length (lengthOfSubstring) to the setOfSubstrings. 
for(int j = 0; j < lengthOfSubstring; j++){
 sb.append(charArray[i + j]);
}
setOfSubstrings.add(sb.toString());

So you're creating a new StringBuilder and then appending each character. This is a complicated way of retrieving the substring between two indices. It also allocates a new StringBuilder each time which isn't really needed.

You can just have

setOfSubstrings.add(aString.substring(i, i + lengthOfSubstring));

In the same way, stringIntersect is a lot more complicated than it should be.

First of all, you are using a label outerForLoop and then using it to break out of the inner loop with break outerForLoop;. This is generally not a good practice generally not a good practice. What this shows is a missing method: what you really want is to make a method isIntersection(set1, set2) that would determine if the two sets have elements in common.

However, you don't even need to write one, since it is already built-in: Collections#disjoint. This method returns true if the two given collections are disjoint, i.e. have no elements in common. Therefore, you can remove your double for loop and just have:

boolean sameSubstringInBothStrings = !Collections.disjoint(setOne, setTwo);

###Don't add unnecessary pre-checks

Your method getSubstringsSet starts with two early returns:

if(aString.length() < lengthOfSubstring){
 return setOfSubstrings;
}
if(aString.length() == lengthOfSubstring){
 setOfSubstrings.add(aString);
 return setOfSubstrings;
}

You actually don't need them. The rest of the code handles both cases just fine and it doesn't add to clarity or improve performance. Early-returns should be used when the rest of the code can then assume a condition that was checked, making its code simpler; but it is not the case here.

###Use built-in methods

To add the substrings, you are currently doing:

StringBuilder sb = new StringBuilder();
//add each substring of length (lengthOfSubstring) to the setOfSubstrings. 
for(int j = 0; j < lengthOfSubstring; j++){
 sb.append(charArray[i + j]);
}
setOfSubstrings.add(sb.toString());

So you're creating a new StringBuilder and then appending each character. This is a complicated way of retrieving the substring between two indices. It also allocates a new StringBuilder each time which isn't really needed.

You can just have

setOfSubstrings.add(aString.substring(i, i + lengthOfSubstring));

In the same way, stringIntersect is a lot more complicated than it should be.

First of all, you are using a label outerForLoop and then using it to break out of the inner loop with break outerForLoop;. This is generally not a good practice. What this shows is a missing method: what you really want is to make a method isIntersection(set1, set2) that would determine if the two sets have elements in common.

However, you don't even need to write one, since it is already built-in: Collections#disjoint. This method returns true if the two given collections are disjoint, i.e. have no elements in common. Therefore, you can remove your double for loop and just have:

boolean sameSubstringInBothStrings = !Collections.disjoint(setOne, setTwo);

###Don't add unnecessary pre-checks

Your method getSubstringsSet starts with two early returns:

if(aString.length() < lengthOfSubstring){
 return setOfSubstrings;
}
if(aString.length() == lengthOfSubstring){
 setOfSubstrings.add(aString);
 return setOfSubstrings;
}

You actually don't need them. The rest of the code handles both cases just fine and it doesn't add to clarity or improve performance. Early-returns should be used when the rest of the code can then assume a condition that was checked, making its code simpler; but it is not the case here.

###Use built-in methods

To add the substrings, you are currently doing:

StringBuilder sb = new StringBuilder();
//add each substring of length (lengthOfSubstring) to the setOfSubstrings. 
for(int j = 0; j < lengthOfSubstring; j++){
 sb.append(charArray[i + j]);
}
setOfSubstrings.add(sb.toString());

So you're creating a new StringBuilder and then appending each character. This is a complicated way of retrieving the substring between two indices. It also allocates a new StringBuilder each time which isn't really needed.

You can just have

setOfSubstrings.add(aString.substring(i, i + lengthOfSubstring));

In the same way, stringIntersect is a lot more complicated than it should be.

First of all, you are using a label outerForLoop and then using it to break out of the inner loop with break outerForLoop;. This is generally not a good practice. What this shows is a missing method: what you really want is to make a method isIntersection(set1, set2) that would determine if the two sets have elements in common.

However, you don't even need to write one, since it is already built-in: Collections#disjoint. This method returns true if the two given collections are disjoint, i.e. have no elements in common. Therefore, you can remove your double for loop and just have:

boolean sameSubstringInBothStrings = !Collections.disjoint(setOne, setTwo);
Source Link
Tunaki
  • 9.3k
  • 1
  • 31
  • 46

###Don't add unnecessary pre-checks

Your method getSubstringsSet starts with two early returns:

if(aString.length() < lengthOfSubstring){
 return setOfSubstrings;
}
if(aString.length() == lengthOfSubstring){
 setOfSubstrings.add(aString);
 return setOfSubstrings;
}

You actually don't need them. The rest of the code handles both cases just fine and it doesn't add to clarity or improve performance. Early-returns should be used when the rest of the code can then assume a condition that was checked, making its code simpler; but it is not the case here.

###Use built-in methods

To add the substrings, you are currently doing:

StringBuilder sb = new StringBuilder();
//add each substring of length (lengthOfSubstring) to the setOfSubstrings. 
for(int j = 0; j < lengthOfSubstring; j++){
 sb.append(charArray[i + j]);
}
setOfSubstrings.add(sb.toString());

So you're creating a new StringBuilder and then appending each character. This is a complicated way of retrieving the substring between two indices. It also allocates a new StringBuilder each time which isn't really needed.

You can just have

setOfSubstrings.add(aString.substring(i, i + lengthOfSubstring));

In the same way, stringIntersect is a lot more complicated than it should be.

First of all, you are using a label outerForLoop and then using it to break out of the inner loop with break outerForLoop;. This is generally not a good practice. What this shows is a missing method: what you really want is to make a method isIntersection(set1, set2) that would determine if the two sets have elements in common.

However, you don't even need to write one, since it is already built-in: Collections#disjoint. This method returns true if the two given collections are disjoint, i.e. have no elements in common. Therefore, you can remove your double for loop and just have:

boolean sameSubstringInBothStrings = !Collections.disjoint(setOne, setTwo);
lang-java

AltStyle によって変換されたページ (->オリジナル) /