Re: A Beginner:Why is my program always returning true?

Enteng <entengk@gmail.com> wrote:


Yes, variable names are a real mater of personal taste. Maybe the ones you
used work better for how you think about your code. I just wanted to give
you something else to think about.


Yes, much better. You found 2 of the 3 bugs I mentioned.

Try:

   subString = ".blowup";

and see what happens to find the third bug.

Now that you found the basic bugs, I'll add a few more comments you can
think about.


You should generally avoid labeled loops. It's a sign your code is too
complex and you should try to restructure it to make it simpler. Complex
code tends to have hidden bugs in it - as yours does.

Though from your other message, it seems you were using this example as a
place where you can use labeled loops - so that's fine for learning how to
do these things. This is a perfect example of where labeled loops are
needed.


This loop needs to scan increasingly higher indexes in the two strings yet
you use a loop variable that's counting down. That's highly counter
intuitive to me.

The outer loop needs to scan forward as well, but yet you used a downward
counting variable there. Why did you use two different style loops to do
the same basic thing?

In general, loops that count down are much harder to understand than loops
that count up. Try not to use them unless you actually need to count down.


Inside your loop, you needed two variables (m and s) to count up. You
didn't need cnt to count down. This is, cnt wasn't used at all inside the
loop. When you need a variable to change inside a loop, it's best to try
and make that variable the one which is controlling the loop.

The way you wrote it, you ended up with three variables changing in this
inner loop. You only needed one to change. The fact that you wrote it so
that three changed at once, only increased the odds that you would make a
mistake and not change all three at once. It's like trying to keep track
of a 3 ring circus - it's hard to do and easy to miss something - as you
did. It only gets worse when you are dealing with much larger and much
more complex code.


I removed the "test" from that break because it wasn't needed. If I see a
labeled break in code, it makes me assume the label was needed, so I assume
it must be breaking out of multiple layers - which would mean, if the rest
of the code was off the screen (which it is for me at this point because of
my comments) I would assume that break was breaking out more than just the
following }:


Here's a simple rewrite to clean up the loops (without fixing your final
bug):

Test:
  for (int m = 0; m < main.length(); m++) {
    for (int s = 0; s < sub.length(); s++) {
      if (main.charAt(m+s) != sub.charAt(s))
        continue Test;
    }
    isItThere = true;
    break;
  }

See how much simpler it becomes when you use only one variable in the inner
loop instead of three? You used 5 loop control variables, and I only used
2. I used the exact same loop structure for both loops, so it's easier to
understand what the second loop is doing after you figure out what the
first loop is doing. Maybe this rewrite will help you see the final bug as
well?

Note that I again put the mainString.length() inside the for loop instead
of using the max variable. For efficiency, it can be wise to use a
variable instead of calling a method for every test of the loop. But the
goal here is always clear bug-free code first, and efficiency second. You
should only sacrifice clear code for efficiency when you are fairly sure
efficiency is going to be an issue.

If the length() method had to scan the string and count the bytes, then
calling it every time in a loop is an efficiency issue. But java strings
don't work that way. They know their length - it's stored in them. So the
length() method is just a simple getter method - once which, for all I
know, might be optimized away by the compiler as a final method and turned
into a direct access of the length of the string. So the efficiency saved
by using the max variable in this case is minimal.

There's also the issue that the test condition might for some loops, be
changing in the loop. And in general, most loops are less likely to have
bugs if you put the end condition test in the loop, instead of caching it
in a variable like "max" and using the saved value. For example, if you
are scanning a list where elements can be removed from the list as you
scan, you will need to test the current size of the list for each iteration
of the loop instead of using the max variable. I you use a max variable,
you are more likely to have a bug in your code. So as a rule, only use
variables for efficiency like that when you are fairly sure the efficiency
will be important for your application.

But again, this is really a style issue on whether you use the max variable
or put the method call in the loop. I think the gain in simplicity is well
worth the minor loss in efficiency.

There are many ways to rewrite this to get rid of the Test: label. Many
rewrites however only make it more complex and I think the real goal should
always be to make the code as simple and clear as possible. A code with a
bug in it that blows up when it's in production is far far worse, than any
code style issue, but keeping the code as simple as possible, is what helps
us make sure there are no bugs in our code.

Lew suggested one rewrite, but he didn't look at the code long enough to
figure out what you were doing because his rewrite isn't even close to
working - it blows up with an exception even more than yours does. Lew is
normally better than that. :)

Here's one way to rewrite and remove the loop label:

  for (int m = 0; m < mainString.length(); m++) {
    int matchCnt = 0;
    for (int s = 0; s < subString.length(); s++) {
      if (mainString.charAt(m+s) == subString.charAt(s))
        matchCnt++;
      else
        break;
    }
    if (matchCnt == subString.length()) {
        isItThere = true;
        break;
    }
  }

I don't think that makes the code better - I think it makes it more complex
and more bug prone (I've not tested the above code so there's a good chance
it has a bug in it :)).

The way I like to clean up code that gets too complex like that it to use
the power of a return statement. You break it up into separate methods and
keep the work of each method simpler (this code has your bug in it as
well):

// is sub located anywhere in main?

boolean contains(String main, String sub)
{
    for (int o = 0; o < main.legnth(); o++) {
        if (match(main, o, sub))
            return true;
    }

    return false;
}

// Is sub located in main at offset?

boolean match(String main, int offset, String sub)
{
    for (int i = 0; i < sub.length(); i++) {
        if (main.charAt(i+offset) != sub.charAt(i))
            return false;
    }

    return true;
}

(I didn't compile or test the above so it could have bugs - other than the
one from your code)

The power of the return statement replaced the "continue Test" from your
code. In general, we use return so much we are very used to what it does
and how it works, where as you almost never see a labeled loop in code
(I've yet to see my first example of it in real Java code).

It's very common in OO code to break your code into many tiny methods like
this where each one only does one very simple task. This makes it easier
for the programmer to be sure the method is doing the task correctly. The
simpler the task of the method, the more likely you will get the code
right. When you try to do multiple tasks in one method, you increase the
odds that you will make a mistake and not see an error.

When we are being lazy, or when we are in a hurry, we will not take the
time to restructure our code to make it clean, we will just keep adding and
adding crap to a method which is already way too larger because it's easier
to add a few lines, than it is to refracture into a simpler and cleaner
design. But doing that will always come back to bite you with a bug you
didn't catch that you never should have let get past you.

I constantly rewrite my code to see if I can find a better way to express
the algorithm. Learning the simplest way to write the code takes time and
practice. It's all part of the art of programming. It's one thing to be
smart enough to make the code work no matter how it's structured. It's
another thing to be able to simplify the code so you can look at it and
know its right without having to test it to death. You can't test quality
into the code.

--
Curt Welch http://CurtWelch.Com/
curt@kcwc.com http://NewsReader.Com/