0
\$\begingroup\$

I wrote a program that receives 2 strings (Input and Search) in the Search string. The sign '+' indicates that if the substring before the '+' exists in the Input string. (The search string cannot start with '+' and there cannot be a '+' followed by another '+').

Can you review it for best coding practices and efficiency?

boolean notgood = false;
boolean break1 = false;
boolean break2 = false;
int counter = 0;

if (search.charAt(0)=='+'||search.charAt(0)=='*') {

    System.out.println("Invalid search striNG.");
    continue; 
} 
//////////////////////////////////////////////
for (i=0; i<search.length() && notgood==false; i++) {

    if (search.charAt(i)!='*' && search.charAt(i)!='+') {
        counter++;
    }
    if (counter == search.length()) {
        System.out.println("Invalid SEARCH string."); 
        notgood=true;
        break;
    }

    if ((search.charAt(i)=='*')||(search.charAt(i)=='+')) {

        if (i!=search.length()-1) {


            if ((search.charAt(i+1)=='*')||(search.charAt(i+1)=='+')) {

                System.out.println("INvalid search string."); 
                notgood=true;
                break1=true;
            }
        } 
    }
}
////////////////////////////////////////////
for (i=0; i<search.length() && !break1; i++) { 

    int c=0;

    if (search.charAt(i)=='+') {      

        String word = search.substring(0,i); 

        for (int j=0; j<input.length() && !break2; j++) { 

            if ((input.charAt(j) == word.charAt(c)) && c<word.length()) { 
                c++;
            }
            if (c>=word.length()) {

                System.out.println("Search string matches input string.");
                break1=true;
                break2=true;

            }
        } 
        if (c<word.length()) {
            System.out.println("Search string doesn't match input string.");
        }
    }

For example, for Input = 'abcd' and Search = 'ab+cd+', the result should be the strings match.

\$\endgroup\$
  • 1
    \$\begingroup\$ Request for clarification: does ab+cd+ indicate that the two substring which should be present are “ab” and “cd” or is it everything to the left of the +, thus “ab” and “abcd” should be contained? In other words, does the filter “to the left of +” stop when it encounters another plus? \$\endgroup\$ – DapperDan Apr 14 at 18:46
  • \$\begingroup\$ I also noticed you check in the beginning for “*”. What does this character mean in the context of your program? \$\endgroup\$ – DapperDan Apr 14 at 18:47
  • 1
    \$\begingroup\$ Welcome to Code Review! Please read the article on How to write a good question in the Help Center. Update the title of your question after reading. Additionally, fix your code so that it's complete, runnable and working. \$\endgroup\$ – AlexV Apr 14 at 19:49
  • \$\begingroup\$ Why notgood==false when you can shorten it to !notgood? Come to think of it, you could invert the semantics and replace notgood with isGood. \$\endgroup\$ – DodgyCodeException Apr 16 at 12:42
  • \$\begingroup\$ The '*' sign means something else in the program you can ignore him, also, thanks to all of your recommendations, it really helped. and DapperDan i hear what you're saying but it works fine for me when i run it. \$\endgroup\$ – Lior Roz Apr 17 at 15:00
0
\$\begingroup\$

1.) Your search only looks for the first substring only (e.g. "ab"), it will never match the others given (e.g. "cd"). Why? Because You wrote:

String word = search.substring(0,i);

That would give us "ab+cd" for the second substring (maybe this is an error and you wanted to grab "cd"?) Anyway, for the first substring found you are setting break1 and therefore exiting the loop. But instead of setting break-variables, you can break directly by writing "break;". This would be faster and makes your code more readable.

2.) Always make tests (write JUnit-Tests) for your code. You can post them, too. In many companies you cannot check in your code without a corresponding test with full code coverage. If you have input="axb" and search="ab+", then it would match! Is this really intended? I think it's an error, and you would have found out if you wrote tests.

3.) Many times you wrote :

((search.charAt(i)=='*')||(search.charAt(i)=='+'))

This is a double DRY (Don't Repeat Yourself, clean code red grade) coding practice violation. Just evaluate search.charAt() once and store it in a variable "currentChar". Then evaluate the whole expression once and store it in a boolean variable "isDelimiter". Your code:

if (search.charAt(i)!='*' && search.charAt(i)!='+') {
    counter++;
}
...
if ((search.charAt(i)=='*')||(search.charAt(i)=='+')) {
...

would be reduced to:

char currentChar = search.charAt(i);
boolean isDelimiter = (currentChar=='*' || currentChar=='+');
if (!isDelimiter) {
    counter++;
}
...
if (isDelimiter) {
...

4.) Generally code is written once, but read on average about 10 times. That's why we have the coding practice to use meaningful names (clean code orange grade, source code conventions). So change "break1" to "isInvalidSeachString", "break2" to "hasNoMatch", "notgood" to "bad", "counter" to "validCharacterCounter" and "c" to "matchingCharacterCounter"

5.) Follow the Single level of abstraction principle.(clean code, orange grade) You are doing too much in a single method. You are verifying the correctness of the search string. Then you are tying to grab each substring and then match each substring. Meaningful names are much better than a meaningless separator like "///////...". Your structure should be following

public boolean isMatchingWholeSearch(String input, String search)
{
  boolean searchIsCorrect = hasNoStartingPlus(search) && hasNoDoublePlus(search);
  if (searchIsCorrect) searchForAllMatches(input, search);  
}
private boolean searchForAllMatches(String input, String search)
{
  for (int i=0, max=search.length(); i<max; i++) { // performance optimized for-loop
      boolean isSubstringSuffix = (search.charAt(i)=='+');
      if (isSubstringSuffix) {
          String substring = getSubstringBefore(search, i);
          boolean isFound = searchForSingleMatch(input, substring);
          if (isFound) return true;
      }
  }
  return false;
}
private boolean searchForSingleMatch(String input, String search)
{
  // here goes your inner loop. 
  ...
}
private boolean hasNoStartingPlus(String search)
{
   ...
}
private boolean hasNoDoublePlus(String search)
{
   ...
}

6.) For not reinventing the wheel, you could use java streams String.chars() with lambda expressions, or String.split() methods and regular expressions for verifying. That would make your code much more readable and very short. But maybe you can only use an old java version. Tell me if you can use a new one, and I'll give you code samples for each.

\$\endgroup\$
  • \$\begingroup\$ Hi, your answer has been extremly helpful, thank you very much, I appreciate it. By the way, in my class, i'm not allowed to use functions/methods because we have not gotten to it yet, but thanks!! \$\endgroup\$ – Lior Roz Apr 17 at 15:03
  • \$\begingroup\$ Although I'm not allowed to use most of what you wrote in '5)', i will save it for later learning as I will go into methods, thanks a lot, you really helped. \$\endgroup\$ – Lior Roz Apr 17 at 16:30

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.