How would I rewrite this to satisfy the code checker?

Discussion in 'Java' started by laredotornado, Nov 4, 2009.

  1. Hi,
    I'm using Java 1.5, Eclipse Galileo on Mac 10.5.6 and the code
    checking plug-in (PMD) is complaining about the below block ...

    BufferedReader reader = new BufferedReader(new InputStreamReader
    (fileStream));
    StringBuilder stringBuf = new StringBuilder();
    String line = null;
    ...
    while ((line = reader.readLine()) != null) {
    stringBuf.append(line + "\n");
    }

    saying, "Avoid assignments in operands". How would I rewrite the
    while loop to make this error go away but achieve the same
    functionality?

    Thanks, - Dave
     
    laredotornado, Nov 4, 2009
    #1
    1. Advertising

  2. laredotornado

    Lew Guest

    laredotornado wrote:
    > I'm using Java 1.5, Eclipse Galileo on Mac 10.5.6 and the code
    > checking plug-in (PMD) is complaining about the below block ...
    >
    >                           BufferedReader reader = new BufferedReader(new InputStreamReader
    > (fileStream));


    Hey, lighten up on the indentation!

    Use a maximum of four spaces per indent level and don't use TAB
    characters for Usenet code posts.

    >                           StringBuilder stringBuf = new StringBuilder();


    Your variable name choice is slightly misleading.

    >                           String line = null;
    >                           ...
    >                                   while ((line = reader.readLine()) != null) {
    >                                           stringBuf.append(line + "\n");
    >                                   }
    >
    > saying, "Avoid assignments in operands".  How would I rewrite the
    > while loop to make this error go away but achieve the same
    > functionality?
    >


    It's not an error, it's a warning and not even a standard warning for
    Java. It's a perfectly legal construct. However, it does elevate the
    scope of the variable 'line' beyond where it should be. Also, the
    assignment of 'null' to it is superfluous. So really your "checker"
    is giving you good advice.

    You could use a 'for' loop.

    for ( String line = reader.readLine(); line != null; line =
    reader.readLine() )
    {
    ...
    }

    Does FindBugs work on the Mac?

    --
    Lew
     
    Lew, Nov 4, 2009
    #2
    1. Advertising

  3. On Nov 4, 2:21 pm, Lew <> wrote:
    > laredotornado wrote:
    > > I'm using Java 1.5, Eclipse Galileo on Mac 10.5.6 and the code
    > > checking plug-in (PMD) is complaining about the below block ...

    >
    > >                           BufferedReader reader = new BufferedReader(new InputStreamReader
    > > (fileStream));

    >
    > Hey, lighten up on the indentation!
    >
    > Use a maximum of four spaces per indent level and don't use TAB
    > characters for Usenet code posts.
    >
    > >                           StringBuilder stringBuf = new StringBuilder();

    >
    > Your variable name choice is slightly misleading.
    >
    > >                           String line = null;
    > >                           ...
    > >                                   while ((line = reader.readLine()) != null) {
    > >                                           stringBuf.append(line + "\n");
    > >                                   }

    >
    > > saying, "Avoid assignments in operands".  How would I rewrite the
    > > while loop to make this error go away but achieve the same
    > > functionality?

    >
    > It's not an error, it's a warning and not even a standard warning for
    > Java.  It's a perfectly legal construct.  However, it does elevate the
    > scope of the variable 'line' beyond where it should be.  Also, the
    > assignment of 'null' to it is superfluous.  So really your "checker"
    > is giving you good advice.
    >
    > You could use a 'for' loop.
    >
    >  for ( String line = reader.readLine(); line != null; line =
    > reader.readLine() )
    >  {
    >    ...
    >  }
    >
    > Does FindBugs work on the Mac?
    >
    > --
    > Lew


    Sweet! Works like a dream. 5 stars.

    I don't know if FindBugs works on a Mac but there is a plug-in for
    Eclipse and since Eclipse is cross-platform, I assume so, but haven't
    tried FindBugs yet.

    Thanks, -
     
    laredotornado, Nov 4, 2009
    #3
  4. laredotornado wrote:
    > On Nov 4, 2:21 pm, Lew <> wrote:

    [ SNIP ]

    >>
    >> Does FindBugs work on the Mac?
    >>
    >> --
    >> Lew

    >
    > Sweet! Works like a dream. 5 stars.
    >
    > I don't know if FindBugs works on a Mac but there is a plug-in for
    > Eclipse and since Eclipse is cross-platform, I assume so, but haven't
    > tried FindBugs yet.
    >
    > Thanks, -


    FindBugs Eclipse plugin and FindBugs standalone work just fine on Mac OS
    X 10.5/10.6.

    AHS
     
    Arved Sandstrom, Nov 5, 2009
    #4
  5. laredotornado

    Roedy Green Guest

    On Wed, 4 Nov 2009 13:06:20 -0800 (PST), laredotornado
    <> wrote, quoted or indirectly quoted someone
    who said :

    >hile ((line = reader.readLine()) != null) {
    > stringBuf.append(line + "\n");
    > }
    >
    >saying, "Avoid assignments in operands". How would I rewrite the
    >while loop to make this error go away but achieve the same
    >functionality?


    That code is fine. There is really no other way to do it. However in
    general it is confusing to newbies if you write code of the form:

    x = ( a = b ); // = assignment embedded in expression.
    as opposed to
    x = ( a == b );
    --
    Roedy Green Canadian Mind Products
    http://mindprod.com

    An example (complete and annotated) is worth 1000 lines of BNF.
     
    Roedy Green, Nov 5, 2009
    #5
  6. laredotornado

    Tom McGlynn Guest

    On Nov 5, 12:59 am, Roedy Green <>
    wrote:
    > On Wed, 4 Nov 2009 13:06:20 -0800 (PST), laredotornado
    > <> wrote, quoted or indirectly quoted someone
    > who said :
    >
    > >while ((line = reader.readLine()) != null) {
    > > stringBuf.append(line + "\n");
    > >}

    > [code edited for appearance]
    > >saying, "Avoid assignments in operands". How would I rewrite the
    > >while loop to make this error go away but achieve the same
    > >functionality?

    >
    > That code is fine. There is really no other way to do it. However in
    > general it is confusing to newbies if you write code of the form:
    >
    > x = ( a = b ); // = assignment embedded in expression.
    > as opposed to
    > x = ( a == b );
    > --
    > Roedy Green Canadian Mind Productshttp://mindprod.com
    >
    > An example (complete and annotated) is worth 1000 lines of BNF.


    The problem with this idiom for me is that you have a choice of either
    having a statement that has side effects, e.g., the original code, or
    you need to duplicate the read statement. While both of these are
    legal I don't like either. They both seem inelegant. Using the for
    statement as Lew suggested at least makes sure that the two reads are
    close together. This idiom is pretty common and I wonder if it's
    worth considering a little bit of syntax that would allow (imho)
    cleaner code.

    Perhaps something equivalent to the for loop where the action that
    takes place at the bottom of the loop in a standard for loop takes
    place at the top of the loop. I.e.,
    for (a; b; c) {d}
    is equivalent to
    {
    a;
    while (b) {
    d;
    c;
    }
    }

    If we extended the while loop where

    while(a; b; c) {d}

    is rendered as

    {
    a;
    while (1) {
    b;
    if (c) {
    d;
    } else {
    break;
    }
    }
    }

    Then the original request becomes

    while(; line=reader.readLine(); line != null) {
    ...
    }
    which avoids both the statement with side effects and duplication
    of the readLine().

    Of with all three clauses

    while (Reader r = getReader(); line=r.readLine(); line != null) {

    I think I would use such this rather often were it available. Has
    such a construct been implemented in other languages or proposed for
    Java?

    Regards,
    Tom McGlynn
     
    Tom McGlynn, Nov 5, 2009
    #6
  7. laredotornado

    Lew Guest

    Roedy Green wrote:
    > On Wed, 4 Nov 2009 13:06:20 -0800 (PST), laredotornado
    > <> wrote, quoted or indirectly quoted someone
    > who said :
    >
    >> hile ((line = reader.readLine()) != null) {
    >> stringBuf.append(line + "\n");
    >> }
    >>
    >> saying, "Avoid assignments in operands". How would I rewrite the
    >> while loop to make this error go away but achieve the same
    >> functionality?

    >
    > That code is fine. There is really no other way to do it. However in
    > general it is confusing to newbies if you write code of the form:


    The code isn't necessarily fine. It puts the scope of 'line' outside the
    loop, where in most cases it should be confined to the loop.

    There is really another way to do it, posted about eight hours prior to your
    post and quoted by the OP. This other way confines the scope of 'line' to the
    loop.

    --
    Lew
     
    Lew, Nov 5, 2009
    #7
  8. laredotornado

    Lew Guest

    Tom McGlynn wrote:
    > The problem with this idiom for me is that you have a choice of either
    > having a statement that has side effects, e.g., the original code, or
    > you need to duplicate the read statement. While both of these are
    > legal I don't like either. They both seem inelegant.


    "Seem" being the operative term.

    There's nothing wrong with writing the 'readLine()' assignment twice, since
    that is what purchases the scope confinement for the 'line' variable.

    --
    Lew
    Don't quote sigs.
     
    Lew, Nov 5, 2009
    #8
  9. laredotornado

    Tom McGlynn Guest

    On Nov 5, 9:21 am, Lew <> wrote:
    > Tom McGlynn wrote:
    > > The problem with this idiom for me is that you have a choice of either
    > > having a statement that has side effects, e.g., the original code, or
    > > you need to duplicate the read statement. While both of these are
    > > legal I don't like either. They both seem inelegant.

    >
    > "Seem" being the operative term.
    >
    > There's nothing wrong with writing the 'readLine()' assignment twice, since
    > that is what purchases the scope confinement for the 'line' variable.
    >


    For me duplication of code is almost always inelegant and even
    slightly dangerous. There's always the chance that there could be
    unintended inconsistencies between the two instances--especially when
    code gets modified. I make no claim that this is a massive issue, but
    neither is the suggested change very large. As with the for
    statement it allows restricting the scope of the variables used in
    the loop. Given that it's not currently valid it is naturally less
    familiar than currently legal idioms. The particular syntax I used is
    the result of at least 30 seconds of thought: there is likely
    something better, I was interested in the idea not the particular
    implementation.

    The proposal would address a fairly broad class of loops: whenever the
    loop is to be terminated based upon some expression, and the value of
    that expression is also needed within the loop.

    Regards
    Tom McGlynn
     
    Tom McGlynn, Nov 5, 2009
    #9
  10. laredotornado

    Lew Guest

    Lew wrote:
    >> There's nothing wrong with writing the 'readLine()' assignment twice, since
    >> that is what purchases the scope confinement for the 'line' variable.


    Tom McGlynn wrote:
    > For me duplication of code is almost always inelegant and even
    > slightly dangerous. There's always the chance that there could be
    > unintended inconsistencies between the two instances--especially when
    > code gets modified. I make no claim that this is a massive issue, but
    > neither is the suggested change very large.


    Copy-and-paste avoids divergence of the duplicated expressions.

    Since I don't have a problem with the idiom the OP wrote about, I usually do this:

    for ( String line; (line = reader.readLine()) != null; )
    {
    ...
    }

    This limits the scope of 'line', avoids curly-brace explosion and avoids
    unnecessary duplication of code, much like your proposed 'while' syntax except
    that it's legal.

    --
    Lew
     
    Lew, Nov 7, 2009
    #10
  11. laredotornado

    Tom Anderson Guest

    On Fri, 6 Nov 2009, Lew wrote:

    > Lew wrote:
    >>> There's nothing wrong with writing the 'readLine()' assignment twice,
    >>> since
    >>> that is what purchases the scope confinement for the 'line' variable.

    >
    > Tom McGlynn wrote:
    >> For me duplication of code is almost always inelegant and even
    >> slightly dangerous. There's always the chance that there could be
    >> unintended inconsistencies between the two instances--especially when
    >> code gets modified. I make no claim that this is a massive issue, but
    >> neither is the suggested change very large.

    >
    > Copy-and-paste avoids divergence of the duplicated expressions.
    >
    > Since I don't have a problem with the idiom the OP wrote about, I usually do
    > this:
    >
    > for ( String line; (line = reader.readLine()) != null; )
    > {
    > ...
    > }
    >
    > This limits the scope of 'line', avoids curly-brace explosion and avoids
    > unnecessary duplication of code, much like your proposed 'while' syntax
    > except that it's legal.


    I like that.

    If you had this:

    http://urchin.earth.li/~twic/Code/LineIterator.java

    You could write:

    import static LineIterator.lines;

    for (String line: lines(reader)) {
    ...
    }

    However, you would throw away your ability to see exceptions, and there's
    a bit more runtime overhead.

    tom

    --
    Yesterday's research projects are today's utilities and tomorrow's
    historical footnotes. -- Roy Smith
     
    Tom Anderson, Nov 7, 2009
    #11
  12. laredotornado

    Tom McGlynn Guest

    On Nov 7, 7:57 am, Tom Anderson <> wrote:
    > On Fri, 6 Nov 2009, Lew wrote:
    > > Lew wrote:
    > >>> There's nothing wrong with writing the 'readLine()' assignment twice,
    > >>> since
    > >>> that is what purchases the scope confinement for the 'line' variable.

    >
    > > Tom McGlynn wrote:
    > >> For me duplication of code is almost always inelegant and even
    > >> slightly dangerous. There's always the chance that there could be
    > >> unintended inconsistencies between the two instances--especially when
    > >> code gets modified. I make no claim that this is a massive issue, but
    > >> neither is the suggested change very large.

    >
    > > Copy-and-paste avoids divergence of the duplicated expressions.

    >
    > > Since I don't have a problem with the idiom the OP wrote about, I usually do
    > > this:

    >
    > > for ( String line; (line = reader.readLine()) != null; )
    > > {
    > > ...
    > > }

    >
    > > This limits the scope of 'line', avoids curly-brace explosion and avoids
    > > unnecessary duplication of code, much like your proposed 'while' syntax
    > > except that it's legal.

    >
    > I like that.
    >
    > If you had this:
    >
    > http://urchin.earth.li/~twic/Code/LineIterator.java
    >
    > You could write:
    >
    > import static LineIterator.lines;
    >
    > for (String line: lines(reader)) {
    > ...
    >
    > }
    >
    > However, you would throw away your ability to see exceptions, and there's
    > a bit more runtime overhead.
    >
    > tom
    >



    Lew's construction may be the best of the bunch using an expression
    with side effects. But putting it in a for loop doesn't alter the
    fact that there is an expression doing two things as once -- which I
    dislike. This situation is pretty much the only time I write code
    that includes such expressions. At other times I've used the
    duplicate assignment approach -- and occasionally have paid a price
    where I missed updating one of them in later on, though it has usually
    been quickly detected. I still don't like either approach. It sounds
    like Eclipse PMD plugin shares at least some of my prejudices.

    Tom's suggestion of the use of an iterator when it's available is very
    nice but as he points out the handling of exceptions is an issue. For
    me at least this kind of loop comes up in contexts other than I/O.
    I'll spend a little more time thinking about if I can build/use an
    iterator when it seems appropriate. But it is probably overkill in
    most simple cases.

    Tom McGlynn
     
    Tom McGlynn, Nov 7, 2009
    #12
    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. Replies:
    6
    Views:
    305
    John Carson
    Nov 7, 2005
  2. Replies:
    2
    Views:
    340
  3. laredotornado
    Replies:
    6
    Views:
    6,510
  4. Pager O Rama

    MSN BLOCK CHECKER-MSN STATUS CHECKER-MSN PROBLEMS

    Pager O Rama, Apr 4, 2006, in forum: ASP General
    Replies:
    0
    Views:
    277
    Pager O Rama
    Apr 4, 2006
  5. Jacob Grover
    Replies:
    5
    Views:
    326
    Jacob Grover
    Jul 18, 2008
Loading...

Share This Page