java.lang.StringIndexOutOfBounds improvement

Question

public class DuplicateLetters {
    
    // CW2.2 Lab-Group-06 Question 5
    // You are given two non-empty strings source and target, all lowercase letters, from a user.
    // The user intends to type the string given in target.
    // However, due to using an old keyboard,
    // when the user was typing, a number characters may be duplicated, and typed one or more times.
    // The source is what the user actually typed, which may or may not be the intended target.
    // Return true if and only if it is possible that some characters of target have been duplicated in source,
    // while the user intended to type the target string.
    // You must use String methods in lecture notes.
    // You must NOT use StringBuilder or Regular Expression methods.
    
    public static boolean duplicateLetters(String source, String target) {
        boolean test = false;
        
        for(int i=0; i<source.length(); i++) {
            for(int j=i; j<target.length(); j++) {
                if(Character.toString(source.charAt(i)).equals(Character.toString(target.charAt(j)))) {
                    test = true;
                }
                else {
                    test = false;
                    j--;    
                }
                i++;
            }
        }
        return test;
    }
    
    public static void main(String[] args) {
        
        System.out.println(duplicateLetters("andddrew", "andrew"));
        // true, there are duplicated letters typed
        
        System.out.println(duplicateLetters("lllejiiee", "leejie"));
        // false, note the initial letter e
        // there is only one initial e in the source, whereas there is a double initial ee in the target
        
        System.out.println(duplicateLetters("cherry", "cherry"));
        // true, no duplicated letters typed this time
    }

This is my code for that question, but it keeps getting the java.lang.StringIndexOutOfBounds error. So, I want to know what is wrong with my code, and how to improve this.

it means you should iterate one less time. also: don't put result = false; in your else block. otherwise, the only real check is the last char. — Stultuske, Nov 19 '20 at 11:03
Your test code, `if(Character.toString(source.charAt(i)).equals(Character.toString(target.charAt(j))))`, is insanely over-complicated, and doesn’t make much sense. `if (source.charAt(i) == target.charAt(i)) …`. — Konrad Rudolph, Nov 19 '20 at 11:04
@KonradRudolph `source` can be longer than `target`, so `target.charAt(i)` will produce `StringIndexOutOfBounds` again — maksimov, Nov 19 '20 at 11:10
@maksimov Well that was obviously a typo in my comment, I meant `target.charAt(j)`. Either way, the surrounding code is still wrong; I just wanted to show how to compare characters in strings, not fix OP’s unrelated problem. — Konrad Rudolph, Nov 19 '20 at 11:12

MD Ruhul Amin · Answer 1 · 2020-11-19T11:14:45.367

You are incrementing the value i inside inner loop, when the value of i goes beyond the source.length exception occurred.

for(int j=i; j<target.length(); j++) {
            if(Character.toString(source.charAt(i)).equals(Character.toString(target.charAt(j)))) {
                test = true;
            }
            else {
                test = false;
                j--;    // you don't need to decrement the value of j.
            }
            i++; // problematic code.
        }

to fix this add a check before comparing or break the loop. for example:

       ...
       i++; // break the loop so that i remains consistent.
       if(i>=source.length()) break;

Also your logic is too complicated and wrong. As fixing those errors are beyond the scope, so you should find it yourself(Homework) :P.

score 0 · Accepted Answer · answered Nov 19 '20 at 12:10

Here's a correct running version. Note you also need to check if the new character that doesn't match matches the last one of the target. duplicateLetters("axbc", "abc"); would otherwise fail. (This will still happen in the already proposed index fix by ruhul).

    public static boolean duplicateLetters(String source, String target) {
        int i = 0;
        char lastTargetChar = target.charAt(0);
        for(int j=0; j<target.length(); j++) {
            while ( source.charAt(i) != target.charAt(j)) {
                // we only go in here if the char in source is wrong, thus assume a dupe, but check if it's a dupe!
                if (lastTargetChar != source.charAt(i)) {
                    return false;
                }
                i++;
                if (i >= source.length()) {
                    return false;
                }
            }
            // if we end here, source char was found ok, so remember last char and go to next char in source
            lastTargetChar = target.charAt(j);
            i++;
        }
        return true;
    }

Optimisations:

Loop over target for easier check on double letters in target (e.g. "letter" and "lettttter").
Exit the loop immediately when there's an issue. You do not need to proceed, as result will be true in any case.

I want to ask one more thing. If I add boolean test = true; and replace return false; --> test = false; then it still throws the exception. Why is this? — Doyeon.K, Nov 23 '20 at 04:48
return will jump out of the method immediately while assigning a variable does not. So you continue executing the loop and thus an error will be thrown. It's not best practice to have break and return statements all around, as it's not obvious in long code where the method ends, but in this small codeblock this should be good to go. Otherwise there would be more checks needed. Also it's faster this way as you would execute the complete for loop! So a replacement with e.g. a while loop would be the way to go with more checks and so forth. In short: It wouldn't be a drop in replacement. — supernova, Nov 23 '20 at 12:59

java.lang.StringIndexOutOfBounds improvement

2 Answers2