Counting length-2 substrings that are common to two strings at the same offset

Question

I am working through the CodingBat exercises for Java. I just completed this one, an exercise that requests the comparison of substrings of two strings:

Given 2 strings, a and b, return the number of the positions where they contain the same length 2 substring. So "xxcaazz" and "xxbaaz" yields 3, since the "xx", "aa", and "az" substrings appear in the same place in both strings.

stringMatch("xxcaazz", "xxbaaz") → 3 
stringMatch("abc", "abc") → 2 
stringMatch("abc", "axc") → 0

Here is my method (which does work):

public int stringMatch(String a, String b) {
 int count = 0;
 String sub2 = "";
 String arrayA[] = new String[a.length()];
 String arrayB[] = new String[b.length()];
 for (int i = 0; i < a.length()-1; i++) {
 sub2 = a.substring(i, i+2);
 arrayA[i] = sub2; 
 }
 for (int i = 0; i < b.length()-1; i++) {
 sub2 = b.substring(i, i+2);
 arrayB[i] = sub2; 
 }
 for (int j = 0; j < arrayA.length-1; j++) {
 if (j >= b.length())
 break;
 if (arrayA[j].equals(arrayB[j]))
 count++;
 }
 return count;
}

And here is the answer on the site:

public int stringMatch(String a, String b) {
 int len = Math.min(a.length(), b.length());
 int count = 0;
 for (int i=0; i<len-1; i++) {
 String aSub = a.substring(i, i+2);
 String bSub = b.substring(i, i+2);
 if (aSub.equals(bSub)) { // Use .equals() with strings
 count++;
 }
 }
 return count;
}

I feel that my code is longer and messy and drawn out compared to other people's. As a beginner programmer, how should I improve it?

• Code Whisperer: In my opinion, in the beginning you should focus on making the code work and produce the expected results. As you do more exercises you should start focusing on reducing the amount of code required to perform a task. The code consolidation comes with experience and learning. I think it is unreasonable to expect a beginner to be as concise as an experienced programmer. What should be expected from a beginner is the functionality at least. Some training exercises force you to create concise code. As an exercise after you have completed a problem and generated the expected output, you can go back
• JFPicard: If I may give you some pointers here, try to read Clean Code by Robert C. Martin (en.wikipedia.org/wiki/Robert_Cecil_Martin) it gives good pointers about the importance and how to write clean code. You can think about your code as making a first draft (make it works) then make the real one (make it clean)
• bwegs: The most important thing is that you figure it out on your own first (get it working). Once you've done this I'd take the time to analyze the more concise solution and try to understand what they've done differently and make a note of those differences. As you continue to learn you'll become more familiar with the language and built-in functions that make things easier (and look more concise). As a beginner you can't be expected to know that in Java you can use Math.min(x,y) to easily return the lesser of 2 values. These things come with both time and practice. In short - Get it to work
• JB Nizet: The specific code you posted above violates the DRY pinciple: Don't Repeat Yourself. The code used to construct arrayA is exactly the same as the one used to construct arrayB. Extracting this code to a method would immediately make your code more concise, easier to maintain and test. Conciseness is not what you should strive for. Readability, robustness and maintainability are more important. Repeating similar portions of code forces the maintainer to make the same evolutions or bug fixes several times, and increases the potential for bugs.
• Jamal: Reminder: your answer must be a review of the code itself. I've already converted two answers to comments so far, one being link-only.

Answer 1

About your code:

This code:

for (int i = 0; i < a.length()-1; i++) {
 sub2 = a.substring(i, i+2);
 arrayA[i] = sub2; 
}

Is repeated almost exactly when dealing with arrayB. Also, sub2 doesn't really do anything. Fixing just these 2 flaws would already greatly improve your code.

Fixing that and 2 other minor flaws (i instead of j as a loop counter and calculating length instead of bailing out in the middle of the loop sometimes) would get you something like this:

public int stringMatch(String a, String b) {
 int count = 0;
 String arrayA[] = lengthNsubstrings(a, 2);
 String arrayB[] = lengthNsubstrings(b, 2);
 int length = Math.min(arrayA.length,
 arrayB.length);
 for (int i = 0; i < length-1; i++) {
 if (arrayA[i].equals(arrayB[i]))
 count++;
 }
 return count;
}
private String[] lengthNsubstrings(String s, int n) {
 String arrayA[] = new String[s.length()];
 for (int i = 0; i < s.length()-1; i++) {
 arrayA[i] = s.substring(i, i+n);
 }
}

If you took that code and decided to get rid of the intermediate arrays, you'd end up with something almost exactly like their answer.

How to improve:

• You can post more questions here. (I know this was migrated here, but nothing's stopping you from posting requests for reviews here directly.)
• You can keep doing what you were doing: working through an exercise and then comparing your answer to their answer. Try to extract general lessons from your mistakes. The point of doing these exercises isn't to produce perfect code the first time you try, but to learn things.
• When coding, be critical of your own code. You may not be able to catch all of your own mistakes while learning, but you can catch some of them.

General lessons you could learn from this code:

• DRY: Don't Repeat Yourself. (Actually, their answer has a minor violation of this, as they hardcode the number 2 in two different places. If you wanted to change it to make it work with length 3 substrings, you'd have to change it in both spots to make it work.)
• Extract repeated code into methods.
• If your code puts data into a data structure only to immediately pull it back out and doesn't ever use the data structure for anything else, you might be better off just using the data directly. (It's not always useless to do it the way you did it here, but in this case it didn't help.)

Answer 2

If I were you I would try to write more concise solutions. Readability is extremely important. If you write code that is hard to read, it will not be reused as much, it will be hard to change or extend the code, your code will probably perform worse, and it will be hard to find bugs.

Your first approach can be based on your intuition, but then you should look at your code, and try to find ways to simplify it. With time and experience, this will get easier and easier, and your intuitive solution will more closely resemble a concise solution.

Ignoring the complexity of your solution, here are some things that could be better:

• Your first two for loops perform exactly the same functionality, and you shouldn't duplicate code. Whenever you copy-paste a piece of code, think about extracting it to a method. This will increase the readability, maintainability, and reusability of your code.
• define variables when they are needed, not earlier. The fewer variables a reader has to remember at any point, the easier it will be to understand your code. count for example isn't needed at the beginning of the method, but just in the last for loop, so declare it there. Another example is sub2, which isn't needed at all, and which also doesn't have a very expressive name.
• use spaces around - and + to increase readability.
• don't declare variables that are only used once, but use those values directly, eg arrayA[i] = a.substring(i, i+2);.
• use curly brackets even for one line statements to increase readability and possibly avoid future bugs.
• be consistent. Why do you use i as loop variable for the first two loops, but not the third? If there is no reason, use i for all.
• Why have if (j >= b.length())? It's not intuitively clear what it does, so you should at least add a comment.