Grokbase › Groups › Cayenne › dev › October 2007
FAQ
Hi Kevin,

Nice work cleaning up the code tonight!

One question - why do you think the break statement below is not
needed? (Keep in mind that it is past 1am in my TZ right now, so I
may be asking stupid/obvious stuff :-))

Andrus

On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:

Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
======================================================================
========
--- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
(original)
+++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
Oct 14 14:30:39 2007
@@ -159,10 +157,7 @@
t1);
}
finally {
- if (iteratedResult) {
- break;
- }
- else {
+ if (!iteratedResult) {
resultSet.close();
}
}

Search Discussions

  • Kevin Menard at Oct 14, 2007 at 10:28 pm ⇧
    Because it was in a finally block. It appeared as though the break was
    added because returning from a finally is a big no-no. But, the break was
    essentially a noop for that one case. By inverting the logic and dealing
    with only the condition you care about, the other is a de facto noop.

    Hopefully I explained that well enough. It's late for me, too. I'm in my
    German hotel room whittling away time.

    --
    Kevin

    On 10/14/07 6:14 PM, "Andrus Adamchik" wrote:

    Hi Kevin,

    Nice work cleaning up the code tonight!

    One question - why do you think the break statement below is not
    needed? (Keep in mind that it is past 1am in my TZ right now, so I
    may be asking stupid/obvious stuff :-))

    Andrus

    On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:

    Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
    src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
    URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
    cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
    jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
    ======================================================================
    ========
    --- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
    main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
    (original)
    +++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
    main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
    Oct 14 14:30:39 2007
    @@ -159,10 +157,7 @@
    t1);
    }
    finally {
    - if (iteratedResult) {
    - break;
    - }
    - else {
    + if (!iteratedResult) {
    resultSet.close();
    }
    }
    --
  • Andrus Adamchik at Oct 15, 2007 at 7:23 am ⇧
    "break" is not really a noop, as we are dealing with "while(true)"
    loop. This may not be coded well, but if I remember correctly the
    intent was to get out of the loop for iterated results, without
    checking for possible update counts. Sure we should not probably put
    it in "finally".

    Andrus

    On Oct 15, 2007, at 1:27 AM, Kevin Menard wrote:

    Because it was in a finally block. It appeared as though the break
    was
    added because returning from a finally is a big no-no. But, the
    break was
    essentially a noop for that one case. By inverting the logic and
    dealing
    with only the condition you care about, the other is a de facto noop.

    Hopefully I explained that well enough. It's late for me, too.
    I'm in my
    German hotel room whittling away time.

    --
    Kevin

    On 10/14/07 6:14 PM, "Andrus Adamchik" wrote:

    Hi Kevin,

    Nice work cleaning up the code tonight!

    One question - why do you think the break statement below is not
    needed? (Keep in mind that it is past 1am in my TZ right now, so I
    may be asking stupid/obvious stuff :-))

    Andrus

    On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:

    Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
    src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
    URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
    cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
    jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
    ====================================================================
    ==
    ========
    --- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
    main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
    (original)
    +++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
    main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
    Oct 14 14:30:39 2007
    @@ -159,10 +157,7 @@
    t1);
    }
    finally {
    - if (iteratedResult) {
    - break;
    - }
    - else {
    + if (!iteratedResult) {
    resultSet.close();
    }
    }
    --

  • Kevin Menard at Oct 15, 2007 at 8:05 am ⇧
    Then I'd probably say that needs to be cleaned up. I was alerted to the
    potential issue from IntelliJ's code inspector. Then looking at the code,
    it was structured a bit oddly.

    I could understand breaking on an exception, but from a cleanup block that
    runs at a non-deterministic time, it seems very suspect to me. We can
    revert the commit, but it still looks like a place that deserves some amount
    of attention.

    --
    Kevin

    On 10/15/07 3:23 AM, "Andrus Adamchik" wrote:

    "break" is not really a noop, as we are dealing with "while(true)"
    loop. This may not be coded well, but if I remember correctly the
    intent was to get out of the loop for iterated results, without
    checking for possible update counts. Sure we should not probably put
    it in "finally".

    Andrus

    On Oct 15, 2007, at 1:27 AM, Kevin Menard wrote:

    Because it was in a finally block. It appeared as though the break
    was
    added because returning from a finally is a big no-no. But, the
    break was
    essentially a noop for that one case. By inverting the logic and
    dealing
    with only the condition you care about, the other is a de facto noop.

    Hopefully I explained that well enough. It's late for me, too.
    I'm in my
    German hotel room whittling away time.

    --
    Kevin

    On 10/14/07 6:14 PM, "Andrus Adamchik" wrote:

    Hi Kevin,

    Nice work cleaning up the code tonight!

    One question - why do you think the break statement below is not
    needed? (Keep in mind that it is past 1am in my TZ right now, so I
    may be asking stupid/obvious stuff :-))

    Andrus

    On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:

    Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
    src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
    URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
    cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
    jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
    ====================================================================
    ==
    ========
    --- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
    main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
    (original)
    +++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
    main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
    Oct 14 14:30:39 2007
    @@ -159,10 +157,7 @@
    t1);
    }
    finally {
    - if (iteratedResult) {
    - break;
    - }
    - else {
    + if (!iteratedResult) {
    resultSet.close();
    }
    }
    --

    --

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupdev @
categoriescayenne
postedOct 14, '07 at 10:14p
activeOct 15, '07 at 8:05a
posts4
users2
websitecayenne.apache.org

2 users in discussion

Andrus Adamchik: 2 posts Kevin Menard: 2 posts

Content

People

Support

Translate

site design / logo © 2022 Grokbase