A Beginner:Why is my program always returning true?

Discussion in 'Java' started by Enteng, Nov 19, 2007.

  1. Enteng

    Enteng Guest

    Hi I'm learning Java and there seems to be something wrong with my
    program. It's supposed to identify if the substring is in the main
    string. My program always returns true and I'm a bit confused.

    BTW, this is based from the tutorial from sun. I've read the whole
    fundamentals section and try to re-write the examples there after a
    while.

    Anyway here's the code:

    class findInString {
    public static void main(String[] args){
    String mainString = "The quick brown fox jumped over the lazy dog.";
    String subString = "hey";
    boolean isItThere = false;

    int max = mainString.length();
    Test:
    for (int letter = 0; letter < max; letter++){
    int subStringSize = subString.length();
    int subStringCounter = 0;
    int mainStringCounter = letter;

    while (subStringSize-- > 0){
    if (mainString.charAt(mainStringCounter) !=
    subString.charAt(subStringCounter)){
    continue Test;
    }
    }

    isItThere = true;
    break Test;
    }

    System.out.println(isItThere ? "Word found" : "Not found.");
    }
    }


    If you guys see anything that I should improve on (my programming)
    please feel free to comment. I'd appreciate them. Thanks!
     
    Enteng, Nov 19, 2007
    #1
    1. Advertising

  2. Enteng

    Arne Vajhøj Guest

    Enteng wrote:
    > Hi I'm learning Java and there seems to be something wrong with my
    > program. It's supposed to identify if the substring is in the main
    > string. My program always returns true and I'm a bit confused.
    >
    > BTW, this is based from the tutorial from sun. I've read the whole
    > fundamentals section and try to re-write the examples there after a
    > while.
    >
    > Anyway here's the code:
    >
    > class findInString {
    > public static void main(String[] args){
    > String mainString = "The quick brown fox jumped over the lazy dog.";
    > String subString = "hey";
    > boolean isItThere = false;
    >
    > int max = mainString.length();
    > Test:
    > for (int letter = 0; letter < max; letter++){
    > int subStringSize = subString.length();
    > int subStringCounter = 0;
    > int mainStringCounter = letter;
    >
    > while (subStringSize-- > 0){
    > if (mainString.charAt(mainStringCounter) !=
    > subString.charAt(subStringCounter)){
    > continue Test;
    > }
    > }
    >
    > isItThere = true;
    > break Test;
    > }
    >
    > System.out.println(isItThere ? "Word found" : "Not found.");
    > }
    > }
    >
    > If you guys see anything that I should improve on (my programming)
    > please feel free to comment. I'd appreciate them. Thanks!


    class findInString {
    public static void main(String[] args){
    String mainString = "The quick brown fox jumped over the lazy
    dog.";
    String subString = "hey";
    boolean isItThere = mainString.contains(subString);
    System.out.println(isItThere ? "Word found" : "Not found.");
    }
    }

    Arne
     
    Arne Vajhøj, Nov 19, 2007
    #2
    1. Advertising

  3. Enteng

    Jeff Higgins Guest

    Enteng wrote:
    > Hi I'm learning Java and there seems to be something wrong with my
    > program. It's supposed to identify if the substring is in the main
    > string. My program always returns true and I'm a bit confused.
    >
    > BTW, this is based from the tutorial from sun. I've read the whole
    > fundamentals section and try to re-write the examples there after a
    > while.
    >
    > Anyway here's the code:
    >

    class findInString
    {
    public static void main(String[] args)
    {
    System.out.print(args[0].indexOf(args[1]));
    }
    }
    >
    >
    > If you guys see anything that I should improve on (my programming)
    > please feel free to comment. I'd appreciate them. Thanks!
     
    Jeff Higgins, Nov 19, 2007
    #3
  4. Enteng wrote:
    > Hi I'm learning Java and there seems to be something wrong with my
    > program. It's supposed to identify if the substring is in the main
    > string. My program always returns true and I'm a bit confused.

    ....

    If the objective is to find out whether the substring is in the string,
    just call the String method "contains".

    However, if the objective is to learn more about programming, you should
    debug this one.

    I suggest using much shorter test strings. That way, you can work
    through your code with paper and pencil.

    Patricia
     
    Patricia Shanahan, Nov 19, 2007
    #4
  5. Enteng

    Enteng Guest

    On Nov 19, 12:08 pm, Patricia Shanahan <> wrote:
    > Enteng wrote:
    > > Hi I'm learning Java and there seems to be something wrong with my
    > > program. It's supposed to identify if the substring is in the main
    > > string. My program always returns true and I'm a bit confused.

    >
    > ...
    >
    > If the objective is to find out whether the substring is in the string,
    > just call the String method "contains".
    >
    > However, if the objective is to learn more about programming, you should
    > debug this one.
    >
    > I suggest using much shorter test strings. That way, you can work
    > through your code with paper and pencil.
    >
    > Patricia


    Actually yeah, the objective is to learn more about programming.
     
    Enteng, Nov 19, 2007
    #5
  6. Enteng

    Roedy Green Guest

    On Sun, 18 Nov 2007 18:55:23 -0800 (PST), Enteng <>
    wrote, quoted or indirectly quoted someone who said :

    >while (subStringSize-- > 0){
    > if (mainString.charAt(mainStringCounter) !=
    >subString.charAt(subStringCounter)){
    > continue Test;
    > }


    A few thoughts.

    1. "continue" is almost never used in Java. You can usually get rid
    of it by inverting the sense of your condition.

    2. A loop must have something change each time around that has bearing
    on when we quit. In this case what is changing? What are you TRYING to
    compare with what?

    3. code is easier to understand if you put "final" on any variable
    that will not change its value later.

    4. code is usually easier to understand if anything that is computed
    in a loop that would give the same result every time around, should be
    pulled out ahead of the loop and assigned to a final variable. The
    simpler the innermost loop is, the easier the code is to understand
    and the faster it will run.

    5. Your code is missing the comments explaining what it is TRYING to
    do. Without that, there is no way fix it.

    6. if the assumption others made about "contains" is true, try your
    code out with pencil and paper, or peppered with printouts to follow
    it through step by step on a concrete example to see how it goes off
    the rails.

    I am being deliberately vague. I figure you will learn more from just
    some hints that having the solution handed on a plate. I also get the
    feeling you would rather sweat a bit.
    --
    Roedy Green Canadian Mind Products
    The Java Glossary
    http://mindprod.com
     
    Roedy Green, Nov 19, 2007
    #6
  7. Enteng

    Curt Welch Guest

    Enteng <> wrote:
    > Hi I'm learning Java and there seems to be something wrong with my
    > program. It's supposed to identify if the substring is in the main
    > string. My program always returns true and I'm a bit confused.
    >
    > BTW, this is based from the tutorial from sun. I've read the whole
    > fundamentals section and try to re-write the examples there after a
    > while.
    >
    > Anyway here's the code:
    >
    > class findInString {
    > public static void main(String[] args){
    > String mainString = "The quick brown fox jumped over the
    > lazy dog."; String subString = "hey";
    > boolean isItThere = false;
    >
    > int max = mainString.length();
    > Test:
    > for (int letter = 0; letter < max; letter++){
    > int subStringSize = subString.length();
    > int subStringCounter = 0;
    > int mainStringCounter = letter;
    >
    > while (subStringSize-- > 0){
    > if (mainString.charAt(mainStringCounter)
    > != subString.charAt(subStringCounter)){
    > continue Test;
    > }
    > }
    >
    > isItThere = true;
    > break Test;
    > }
    >
    > System.out.println(isItThere ? "Word found" : "Not
    > found."); }
    > }
    >
    > If you guys see anything that I should improve on (my programming)
    > please feel free to comment. I'd appreciate them. Thanks!


    As others have said, you just have a bug in the code. We all make mistakes
    when we write code, and the one most important thing to learn, is how to
    find bugs without help from others. Half of what a programmer must do is
    remove bugs from code. There is nothing odd happening with the language,
    your code simply isn't doing what you want it to do because you made a
    simple mistake.

    When code doesn't do what you expect it to do, look at it carefully and
    think about what it's doing and you will normally be able to find the
    problem.

    You might try working backwards for example. If the program prints out
    "Word found", then what does that tell you? It tells you that isItThere
    must be true. So work backwards from that. How can isItThere be true?
    There's only one place in the code it gets set to true, so you know it must
    have been set there. Keep working backwards to see how that could have
    happened.

    When in doubt, add println() for every step of the code and look at the
    100's of lines of output and see exactly what it's doing. That will allow
    you to be sure all the statements in the code are doing what you expect
    them to do, and it will allow you to see why the program is returning true.

    You actually have 3 bugs in your code. If you fix the first two, you will
    find the third rather quickly.

    As far as general comments, here are a few to think about.

    Try and keep the code as simple, and as concise as possible. Don't use
    variables when you don't really need them, and use as short of name as is
    reasonable for the scope of the variables. Variables with very limited
    scope (like the index in a short loop) should have very short names, like 1
    to 3 characters, local variables in a short method might be 3 to 5 letters
    long. Arguments a big longer, and instance variables even longer.

    If the variable names are too long, it makes it harder to "see" the code in
    the middle of all the long and verbose variables. Here's a rewrite of your
    code without changing the logic using shorter names. Maybe it will help
    you see the error? I would suggest other changes, but I don't want to give
    away what you bugs are until you find them on your own. :)

    class findInString {
    public static void main(String[] args){

    String main = "The quick brown fox jumped over the lazy dog.";
    String sub = "hey";
    boolean found = false;

    next:
    for (int i = 0; i < main.length(); i++) {
    int cnt = sub.length();
    int s = 0;
    int m = i;

    while (cnt-- > 0) {
    if (main.charAt(m) != sub.charAt(s))
    continue next;
    }

    found = true;
    break;
    }

    System.out.println(found ? "Word found" : "Not found.");
    }
    }

    --
    Curt Welch http://CurtWelch.Com/
    http://NewsReader.Com/
     
    Curt Welch, Nov 19, 2007
    #7
  8. Enteng

    Enteng Guest

    On Nov 19, 2:33 pm, (Curt Welch) wrote:
    > Enteng <> wrote:
    > > Hi I'm learning Java and there seems to be something wrong with my
    > > program. It's supposed to identify if the substring is in the main
    > > string. My program always returns true and I'm a bit confused.

    >
    > > BTW, this is based from the tutorial from sun. I've read the whole
    > > fundamentals section and try to re-write the examples there after a
    > > while.

    >
    > > Anyway here's the code:

    >
    > > class findInString {
    > > public static void main(String[] args){
    > > String mainString = "The quick brown fox jumped over the
    > > lazy dog."; String subString = "hey";
    > > boolean isItThere = false;

    >
    > > int max = mainString.length();
    > > Test:
    > > for (int letter = 0; letter < max; letter++){
    > > int subStringSize = subString.length();
    > > int subStringCounter = 0;
    > > int mainStringCounter = letter;

    >
    > > while (subStringSize-- > 0){
    > > if (mainString.charAt(mainStringCounter)
    > > != subString.charAt(subStringCounter)){
    > > continue Test;
    > > }
    > > }

    >
    > > isItThere = true;
    > > break Test;
    > > }

    >
    > > System.out.println(isItThere ? "Word found" : "Not
    > > found."); }
    > > }

    >
    > > If you guys see anything that I should improve on (my programming)
    > > please feel free to comment. I'd appreciate them. Thanks!

    >
    > As others have said, you just have a bug in the code. We all make mistakes
    > when we write code, and the one most important thing to learn, is how to
    > find bugs without help from others. Half of what a programmer must do is
    > remove bugs from code. There is nothing odd happening with the language,
    > your code simply isn't doing what you want it to do because you made a
    > simple mistake.
    >
    > When code doesn't do what you expect it to do, look at it carefully and
    > think about what it's doing and you will normally be able to find the
    > problem.
    >
    > You might try working backwards for example. If the program prints out
    > "Word found", then what does that tell you? It tells you that isItThere
    > must be true. So work backwards from that. How can isItThere be true?
    > There's only one place in the code it gets set to true, so you know it must
    > have been set there. Keep working backwards to see how that could have
    > happened.
    >
    > When in doubt, add println() for every step of the code and look at the
    > 100's of lines of output and see exactly what it's doing. That will allow
    > you to be sure all the statements in the code are doing what you expect
    > them to do, and it will allow you to see why the program is returning true.
    >
    > You actually have 3 bugs in your code. If you fix the first two, you will
    > find the third rather quickly.
    >
    > As far as general comments, here are a few to think about.
    >
    > Try and keep the code as simple, and as concise as possible. Don't use
    > variables when you don't really need them, and use as short of name as is
    > reasonable for the scope of the variables. Variables with very limited
    > scope (like the index in a short loop) should have very short names, like 1
    > to 3 characters, local variables in a short method might be 3 to 5 letters
    > long. Arguments a big longer, and instance variables even longer.
    >
    > If the variable names are too long, it makes it harder to "see" the code in
    > the middle of all the long and verbose variables. Here's a rewrite of your
    > code without changing the logic using shorter names. Maybe it will help
    > you see the error? I would suggest other changes, but I don't want to give
    > away what you bugs are until you find them on your own. :)
    >
    > class findInString {
    > public static void main(String[] args){
    >
    > String main = "The quick brown fox jumped over the lazy dog.";
    > String sub = "hey";
    > boolean found = false;
    >
    > next:
    > for (int i = 0; i < main.length(); i++) {
    > int cnt = sub.length();
    > int s = 0;
    > int m = i;
    >
    > while (cnt-- > 0) {
    > if (main.charAt(m) != sub.charAt(s))
    > continue next;
    > }
    >
    > found = true;
    > break;
    > }
    >
    > System.out.println(found ? "Word found" : "Not found.");
    > }
    >
    > }
    >
    > --
    > Curt Welch http://CurtWelch.Com/
    > http://NewsReader.Com/


    Thanks for the comments and suggestions. I'm learning a lot :)
    I thought giving variable detailed names would help me understand the
    code better. Anyway I think I got it.

    class findInString {
    public static void main(String[] args){
    String mainString = "The quick brown fox jumped over the lazy dog.";
    String subString = "hey";
    boolean isItThere = false;

    int max = mainString.length();
    Test:
    for (int i = 0; i < max; i++){
    int cnt = subString.length();
    int s = 0;
    int m = i;

    while (cnt-- != 0){
    if (mainString.charAt(m++) != subString.charAt(s++)){
    continue Test;
    }
    }

    isItThere = true;
    break Test;
    }

    System.out.println(isItThere ? "Word found" : "Not found.");
    }
    }
     
    Enteng, Nov 19, 2007
    #8
  9. Enteng

    Lew Guest

    Enteng wrote:
    > Thanks for the comments and suggestions. I'm learning a lot :)
    > I thought giving variable detailed names would help me understand the
    > code better. Anyway I think I got it.
    >
    > class findInString {


    Class names should begin with an upper-case letter.

    > public static void main(String[] args){


    Do NOT use TAB characters in Usenet listings; use spaces to indent (2, 3, or 4
    spaces per level).

    > String mainString = "The quick brown fox jumped over the lazy dog.";


    // "String" is not a good name part for variables.
    // It ties the variable to the physical implementation too tightly.

    > String subString = "hey";
    > boolean isItThere = false;


    > Test:

    /*
    Don't use labels to continue loops. 'continue' by itself is enough, or
    better, follow Roedy's suggestion and invert the condition to eliminate the
    'continue'. (And if you ask for advice, you should consider it before
    rejecting it.)
    */

    > for (int i = 0, max = mainString.length(); i < max; i++){
    > int s = 0;
    > int m = i;
    >

    for ( int cnt = subString.length(); cnt-- != 0 && ! isItThere; ){
    if (mainString.charAt(m++) == subString.charAt(s++)){
    isItThere = true;
    > }
    > }
    > }
    >
    > System.out.println(isItThere ? "Word found" : "Not found.");
    > }
    > }


    --
    Lew
     
    Lew, Nov 19, 2007
    #9
  10. Enteng

    Enteng Guest

    On Nov 19, 11:33 pm, Lew <> wrote:
    > Enteng wrote:
    > > Thanks for the comments and suggestions. I'm learning a lot :)
    > > I thought giving variable detailed names would help me understand the
    > > code better. Anyway I think I got it.

    >
    > > class findInString {

    >
    > Class names should begin with an upper-case letter.
    >
    > > public static void main(String[] args){

    >
    > Do NOT use TAB characters in Usenet listings; use spaces to indent (2, 3, or 4
    > spaces per level).
    >
    > > String mainString = "The quick brown fox jumped over the lazy dog.";

    >
    > // "String" is not a good name part for variables.
    > // It ties the variable to the physical implementation too tightly.
    >
    > > String subString = "hey";
    > > boolean isItThere = false;
    > > Test:

    >
    > /*
    > Don't use labels to continue loops. 'continue' by itself is enough, or
    > better, follow Roedy's suggestion and invert the condition to eliminate the
    > 'continue'. (And if you ask for advice, you should consider it before
    > rejecting it.)
    > */
    >
    > > for (int i = 0, max = mainString.length(); i < max; i++){
    > > int s = 0;
    > > int m = i;

    >
    > for ( int cnt = subString.length(); cnt-- != 0 && ! isItThere; ){
    > if (mainString.charAt(m++) == subString.charAt(s++)){
    > isItThere = true;
    >
    > > }
    > > }
    > > }

    >
    > > System.out.println(isItThere ? "Word found" : "Not found.");
    > > }
    > > }

    >
    > --
    > Lew



    OK thanks I'll take note of that. About Roedy's suggestion I just
    haven't tried some of it yet because I'm making this code while I'm on
    the topic of labeled breaks (if that's what you're pertaining to about
    rejecting advice). Actually it was one of his advices that made me
    figure this out :) Lew, you sound offended or it's just the way you
    give advice?

    Honestly speaking it's kind of hard being criticized. I'm saying this
    not because of your comments and suggestions(which are actually
    helpful and I'm thankful for) but it's just that some comments are
    well, discouraging. Don't get me wrong I appreciate all of them. Maybe
    I'm just not used to this. Anyway I'm OT now, just voicing out :)
     
    Enteng, Nov 19, 2007
    #10
  11. Enteng

    Lew Guest

    Enteng wrote:
    > Honestly speaking it's kind of hard being criticized. I'm saying this
    > not because of your comments and suggestions(which are actually
    > helpful and I'm thankful for) but it's just that some comments are
    > well, discouraging. Don't get me wrong I appreciate all of them. Maybe
    > I'm just not used to this. Anyway I'm OT now, just voicing out :)


    I did not intend to criticize you. This is a discussion group; when something
    that was mentioned was not answered, I suspected that it had been overlooked,
    is all.

    As for the detailed comments, those are all for your study and perusal. They
    aren't criticisms, just comments.

    I do not know why people have to take technical information personally. This
    is a Java newsgroup where people make comments about Java code. That's life
    here in the newsgroup. Get used to it.

    --
    Lew
     
    Lew, Nov 19, 2007
    #11
  12. Enteng wrote:
    ....
    > Thanks for the comments and suggestions. I'm learning a lot :)
    > I thought giving variable detailed names would help me understand the
    > code better. Anyway I think I got it.

    ....

    I agree with many of the comments, but want to comment on a different
    issue. In your code, you have a main method that does three jobs:

    1. Define the tests case(s) to be run.

    2. Report results of a test.

    3. Do the actual search.

    That structure makes it difficult to do multiple tests without editing
    the program. Before doing anything to the algorithm, if it were my
    program I would have done this refactoring:

    public class FindInString {
    public static void main(String[] args) {
    test("The quick brown fox jumped over the lazy dog.",
    "hey", false);
    test("The quick brown fox jumped over the lazy dog.",
    "", true);
    test("The quick brown fox jumped over the lazy dog.",
    "over", true);
    test("The quick brown fox jumped over the lazy dog.",
    "g. ", false);
    }

    private static void test(String mainString,
    String subString, boolean expected) {
    boolean isItThere = substringIsThere(mainString,
    subString);

    if (expected == isItThere) {
    System.out.print("GOOD");
    } else {
    System.out.print("*BAD");
    }
    System.out.println(" " + isItThere + " |" + subString
    + "| |" + mainString + "|");
    }

    private static boolean substringIsThere(
    String mainString, String subString) {
    boolean isItThere;
    isItThere = false;

    int max = mainString.length();
    Test: for (int i = 0; i < max; i++) {
    int cnt = subString.length();
    int s = 0;
    int m = i;

    while (cnt-- != 0) {
    if (mainString.charAt(m++) != subString.charAt(s++)) {
    continue Test;
    }
    }

    isItThere = true;
    break Test;
    }
    return isItThere;
    }
    }

    In this organization, the main method just selects the tests to be run.
    The test method does a single test. The "expected" parameter allows a
    different print-out depending on whether the algorithm worked or not.
    Finally, the substringIsThere method does nothing but find out whether
    the substring appears in the string. Test cases can be added by
    inserting "test" calls in main.

    One of my test cases threw an exception. I put it in there because it is
    the sort of thing that can go wrong in substring searches.

    Patricia
     
    Patricia Shanahan, Nov 19, 2007
    #12
  13. Enteng

    Curt Welch Guest

    Enteng <> wrote:
    > On Nov 19, 2:33 pm, (Curt Welch) wrote:


    > Thanks for the comments and suggestions. I'm learning a lot :)
    > I thought giving variable detailed names would help me understand the
    > code better.


    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.

    > Anyway I think I got it.


    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.

    > class findInString {
    > public static void main(String[] args){
    > String mainString = "The quick brown fox jumped over the
    > lazy dog."; String subString = "hey";
    > boolean isItThere = false;
    >
    > int max = mainString.length();
    > Test:


    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.

    > for (int i = 0; i < max; i++){
    > int cnt = subString.length();
    > int s = 0;
    > int m = i;
    >
    > while (cnt-- != 0){


    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.

    > if (mainString.charAt(m++) !=
    > subString.charAt(s++)){


    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.


    > continue Test;
    > }
    > }
    >
    > isItThere = true;
    > break Test;


    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 }:

    > }
    >
    > System.out.println(isItThere ? "Word found" : "Not
    > found."); }
    > }


    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/
    http://NewsReader.Com/
     
    Curt Welch, Nov 19, 2007
    #13
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Siemel Naran

    Does true ^ true return false?

    Siemel Naran, Jun 17, 2004, in forum: C++
    Replies:
    19
    Views:
    670
    Chris Theis
    Jun 18, 2004
  2. Mr. SweatyFinger

    why why why why why

    Mr. SweatyFinger, Nov 28, 2006, in forum: ASP .Net
    Replies:
    4
    Views:
    917
    Mark Rae
    Dec 21, 2006
  3. Mr. SweatyFinger
    Replies:
    2
    Views:
    2,030
    Smokey Grindel
    Dec 2, 2006
  4. bdb112
    Replies:
    45
    Views:
    1,354
    jazbees
    Apr 29, 2009
  5. Tang Heng
    Replies:
    1
    Views:
    723
    Bob Barrows
    Aug 26, 2009
Loading...

Share This Page