Thoughts on function

Discussion in 'C++' started by Ebenezer, Sep 15, 2011.

  1. Ebenezer

    Ebenezer Guest

    Shalom

    How to improve this function?

    bool
    SendBuffer::Flush ()
    {
    uint32_t const all = buf_.size();
    uint32_t numWritten = 0;
    if (partialFlush_) {
    ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    segment =
    segmented_iterator<unsigned char,
    chunk_size>(buf_.begin()).segment();

    numWritten = Write(sock_, &buf_[0], segment.size());
    if (numWritten < segment.size()) goto ending;
    partialFlush_ = false;
    }

    {
    int32_t const chunks = (all - numWritten) / chunk_size;
    for (int32_t ii = 0; ii < chunks; ++ii) {
    uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
    numWritten += bytes;
    if (bytes < chunk_size) goto ending;
    }
    }

    if (numWritten < all) {
    numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
    }

    ending:
    buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    if (numWritten == all) return true;

    partialFlush_ = true;
    return false;
    }

    Tia.

    Brian Wood
    Ebenezer Enterprises
    http://webEbenezer.net
     
    Ebenezer, Sep 15, 2011
    #1
    1. Advertising

  2. Ebenezer

    Ian Collins Guest

    On 09/15/11 05:01 PM, Ebenezer wrote:
    >
    > Shalom
    >
    > How to improve this function?
    >
    > bool
    > SendBuffer::Flush ()
    > {
    > uint32_t const all = buf_.size();
    > uint32_t numWritten = 0;


    You're probably better off using size_t unless you have a really really
    good reason to use uint32_t.

    > if (partialFlush_) {
    > ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > segment =
    > segmented_iterator<unsigned char,
    > chunk_size>(buf_.begin()).segment();
    >
    > numWritten = Write(sock_,&buf_[0], segment.size());


    Undeclared function again.

    > if (numWritten< segment.size()) goto ending;


    Ug.

    > partialFlush_ = false;


    Lots of undefined stuff there, so can't comment.

    > }
    >
    > {
    > int32_t const chunks = (all - numWritten) / chunk_size;
    > for (int32_t ii = 0; ii< chunks; ++ii) {
    > uint32_t bytes = Write(sock_,&buf_[numWritten], chunk_size);
    > numWritten += bytes;
    > if (bytes< chunk_size) goto ending;


    Ug, why on earth did you go and spoil it by using goto?

    --
    Ian Collins
     
    Ian Collins, Sep 15, 2011
    #2
    1. Advertising

  3. Ebenezer

    Ebenezer Guest

    On Sep 15, 12:08 am, Ian Collins <> wrote:
    > On 09/15/11 05:01 PM, Ebenezer wrote:
    >
    >
    >
    > > Shalom

    >
    > > How to improve this function?

    >
    > > bool
    > > SendBuffer::Flush ()
    > > {
    > >    uint32_t const all = buf_.size();
    > >    uint32_t numWritten = 0;

    >
    > You're probably better off using size_t unless you have a really really
    > good reason to use uint32_t.
    >
    > >    if (partialFlush_) {
    > >      ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > > segment =
    > >                 segmented_iterator<unsigned char,
    > > chunk_size>(buf_.begin()).segment();

    >
    > >      numWritten = Write(sock_,&buf_[0], segment.size());

    >
    > Undeclared function again.
    >
    > >      if (numWritten<  segment.size()) goto ending;

    >
    > Ug.
    >
    > >      partialFlush_ = false;

    >
    > Lots of undefined stuff there, so can't comment.


    Sorry. Some are here --
    http://webEbenezer.net/misc/SendBuffer.hh .
    The Write function is here
    http://webEbenezer.net/misc/IO.cc .
     
    Ebenezer, Sep 15, 2011
    #3
  4. Ebenezer

    Ian Collins Guest

    On 09/16/11 02:34 AM, Ebenezer wrote:
    > On Sep 15, 12:08 am, Ian Collins<> wrote:
    >> On 09/15/11 05:01 PM, Ebenezer wrote:
    >>
    >>
    >>
    >>> Shalom

    >>
    >>> How to improve this function?

    >>
    >>> bool
    >>> SendBuffer::Flush ()
    >>> {
    >>> uint32_t const all = buf_.size();
    >>> uint32_t numWritten = 0;

    >>
    >> You're probably better off using size_t unless you have a really really
    >> good reason to use uint32_t.
    >>
    >>> if (partialFlush_) {
    >>> ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    >>> segment =
    >>> segmented_iterator<unsigned char,
    >>> chunk_size>(buf_.begin()).segment();

    >>
    >>> numWritten = Write(sock_,&buf_[0], segment.size());

    >>
    >> Undeclared function again.
    >>
    >>> if (numWritten< segment.size()) goto ending;

    >>
    >> Ug.
    >>
    >>> partialFlush_ = false;

    >>
    >> Lots of undefined stuff there, so can't comment.

    >
    > Sorry. Some are here --
    > http://webEbenezer.net/misc/SendBuffer.hh .
    > The Write function is here
    > http://webEbenezer.net/misc/IO.cc .


    But that doesn't excuse the gratuitous use of goto!

    --
    Ian Collins
     
    Ian Collins, Sep 15, 2011
    #4
  5. Ebenezer

    Ebenezer Guest

    On Sep 15, 2:38 pm, Ian Collins <> wrote:
    > On 09/16/11 02:34 AM, Ebenezer wrote:
    >
    >
    >
    >
    >
    >
    >
    >
    >
    > > On Sep 15, 12:08 am, Ian Collins<>  wrote:
    > >> On 09/15/11 05:01 PM, Ebenezer wrote:

    >
    > >>> Shalom

    >
    > >>> How to improve this function?

    >
    > >>> bool
    > >>> SendBuffer::Flush ()
    > >>> {
    > >>>     uint32_t const all = buf_.size();
    > >>>     uint32_t numWritten = 0;

    >
    > >> You're probably better off using size_t unless you have a really really
    > >> good reason to use uint32_t.

    >
    > >>>     if (partialFlush_) {
    > >>>       ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > >>> segment =
    > >>>                  segmented_iterator<unsigned char,
    > >>> chunk_size>(buf_.begin()).segment();

    >
    > >>>       numWritten = Write(sock_,&buf_[0], segment.size());

    >
    > >> Undeclared function again.

    >
    > >>>       if (numWritten<    segment.size()) goto ending;

    >
    > >> Ug.

    >
    > >>>       partialFlush_ = false;

    >
    > >> Lots of undefined stuff there, so can't comment.

    >
    > > Sorry.  Some are here --
    > >http://webEbenezer.net/misc/SendBuffer.hh.
    > > The Write function is here
    > >http://webEbenezer.net/misc/IO.cc.

    >
    > But that doesn't excuse the gratuitous use of goto!
    >


    I started by asking for ideas on how to improve it.
    Perhaps someone would show what they believe to be
    an improvement. I realized after thinking about
    your earlier comments that the member variable
    partialFlush could be a static local variable.


    bool
    SendBuffer::Flush ()
    {
    uint32_t const all = buf_.size();
    uint32_t numWritten = 0;
    static bool partialFlush = false;
    if (partialFlush) {
    ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    segment =
    segmented_iterator<unsigned char,
    chunk_size>(buf_.begin()).segment();

    numWritten = Write(sock_, &buf_[0], segment.size());
    if (numWritten < segment.size()) goto ending;
    partialFlush = false;
    }

    {
    int32_t const chunks = (all - numWritten) / chunk_size;
    for (int32_t ii = 0; ii < chunks; ++ii) {
    uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
    numWritten += bytes;
    if (bytes < chunk_size) goto ending;
    }
    }

    if (numWritten < all) {
    numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
    }

    ending:
    buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    if (numWritten == all) return true;

    partialFlush = true;
    return false;
    }
     
    Ebenezer, Sep 16, 2011
    #5
  6. Hi Brian,
    I'm not really an expert, but here a couple of suggestion

    > bool
    > SendBuffer::Flush ()
    > {
    >   uint32_t const all = buf_.size();
    >   uint32_t numWritten = 0;


    use std::size_t instead of uint32_t, it's more portable!

    >   if (partialFlush_) {
    >     ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > segment =
    >                segmented_iterator<unsigned char,
    > chunk_size>(buf_.begin()).segment();
    >
    >     numWritten = Write(sock_, &buf_[0], segment.size());
    >     if (numWritten < segment.size()) goto ending;
    >     partialFlush_ = false;
    >   }


    is partialFlush_ a flag about last flush operation and not the
    current? That is, if a flush doesn't complete for any reason, you set
    partial flush to true, and the next time flush is called, you send the
    pending bytes (in fact, the entire segment that contains the first
    byte in the buffer), am I right?
    Maybe I can't see how useful this is 'cause I have not read all the
    code, but if flush simply try to send the entire buffer, and if it
    cannot, simply erase only the sended part and leave the rest to the
    next call?

    >
    >   {
    >     int32_t const chunks = (all - numWritten) / chunk_size;
    >     for (int32_t ii = 0; ii < chunks; ++ii) {
    >       uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
    >       numWritten += bytes;
    >       if (bytes < chunk_size) goto ending;
    >     }
    >   }


    Maybe the outer enclosing clock could be eliminated, it's not useful
    here.



    >
    >   if (numWritten < all) {
    >     numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
    >   }
    >
    > ending:
    >   buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    >   if (numWritten == all) return true;
    >
    >   partialFlush_ = true;
    >   return false;
    >
    > }


    goto is considered worst practice :) maybe a redesign to eliminate it
    could be better.

    Bye,
    Fulvio Esposito
     
    Fulvio Esposito, Sep 16, 2011
    #6
  7. Ebenezer

    Ebenezer Guest

    On Sep 16, 4:19 am, Fulvio Esposito <> wrote:
    > Hi Brian,
    > I'm not really an expert, but here a couple of suggestion
    >
    > > bool
    > > SendBuffer::Flush ()
    > > {
    > >   uint32_t const all = buf_.size();
    > >   uint32_t numWritten = 0;

    >
    > use std::size_t instead of uint32_t, it's more portable!
    >
    > >   if (partialFlush_) {
    > >     ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > > segment =
    > >                segmented_iterator<unsigned char,
    > > chunk_size>(buf_.begin()).segment();

    >
    > >     numWritten = Write(sock_, &buf_[0], segment.size());
    > >     if (numWritten < segment.size()) goto ending;
    > >     partialFlush_ = false;
    > >   }

    >
    > is partialFlush_ a flag about last flush operation and not the
    > current?


    Yes, it is about the previous flush call.

    > That is, if a flush doesn't complete for any reason, you set
    > partial flush to true, and the next time flush is called, you send the
    > pending bytes (in fact, the entire segment that contains the first
    > byte in the buffer), am I right?
    > Maybe I can't see how useful this is 'cause I have not read all the
    > code, but if flush simply try to send the entire buffer, and if it
    > cannot, simply erase only the sended part and leave the rest to the
    > next call?
    >


    The partialFlush stuff is there to detect if there's a
    need to do some extra work on the first segment. This
    function is used with a marshalling library... we only
    add to the end of the container. So the only way the
    first segment isn't full is if it also the last segment.



    >
    >
    > >   {
    > >     int32_t const chunks = (all - numWritten) / chunk_size;
    > >     for (int32_t ii = 0; ii < chunks; ++ii) {
    > >       uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
    > >       numWritten += bytes;
    > >       if (bytes < chunk_size) goto ending;
    > >     }
    > >   }

    >
    > Maybe the outer enclosing clock could be eliminated, it's not useful
    > here.
    >
    >
    >
    > >   if (numWritten < all) {
    > >     numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
    > >   }

    >
    > > ending:
    > >   buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    > >   if (numWritten == all) return true;

    >
    > >   partialFlush_ = true;
    > >   return false;

    >
    > > }

    >
    > goto is considered worst practice :) maybe a redesign to eliminate it
    > could be better.
    >


    Thanks for your comments. I'm not sure about a redesign.
    That's why I asked.
     
    Ebenezer, Sep 16, 2011
    #7
  8. > Thanks for your comments. I'm not sure about a redesign.
    > That's why I asked.


    mmm maybe I'm missing something, but why you are treating as error a write that send less bytes than what you asked? Why don't you use your PersistentWrite to make sure to send the entire buffer? In general a flush on a stream has the responsibility to empty the buffer sending the entire content to the socket, so its postcondition should be:

    assert( buf_.empty() == true );

    Why you allow partial flushing, exposing logic to clients of SendBuffer class? For example, the sync() method on standard streambuf have this semantic..

    Obviously I don't know much about the context, so partial flush could be a desired behaviour.

    Hope this help!

    Cheers,
    Fulvio
     
    Fulvio Esposito, Sep 16, 2011
    #8
  9. Ebenezer

    Ebenezer Guest

    On Sep 15, 7:23 pm, Ebenezer <> wrote:
    > On Sep 15, 2:38 pm, Ian Collins <> wrote:
    >
    >
    >
    >
    >
    >
    >
    >
    >
    > > On 09/16/11 02:34 AM, Ebenezer wrote:

    >
    > > > On Sep 15, 12:08 am, Ian Collins<>  wrote:
    > > >> On 09/15/11 05:01 PM, Ebenezer wrote:

    >
    > > >>> Shalom

    >
    > > >>> How to improve this function?

    >
    > > >>> bool
    > > >>> SendBuffer::Flush ()
    > > >>> {
    > > >>>     uint32_t const all = buf_.size();
    > > >>>     uint32_t numWritten = 0;

    >
    > > >> You're probably better off using size_t unless you have a really really
    > > >> good reason to use uint32_t.

    >
    > > >>>     if (partialFlush_) {
    > > >>>       ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > > >>> segment =
    > > >>>                  segmented_iterator<unsigned char,
    > > >>> chunk_size>(buf_.begin()).segment();

    >
    > > >>>       numWritten = Write(sock_,&buf_[0], segment.size());

    >
    > > >> Undeclared function again.

    >
    > > >>>       if (numWritten<    segment.size()) goto ending;

    >
    > > >> Ug.

    >
    > > >>>       partialFlush_ = false;

    >
    > > >> Lots of undefined stuff there, so can't comment.

    >
    > > > Sorry.  Some are here --
    > > >http://webEbenezer.net/misc/SendBuffer.hh.
    > > > The Write function is here
    > > >http://webEbenezer.net/misc/IO.cc.

    >
    > > But that doesn't excuse the gratuitous use of goto!

    >
    > I started by asking for ideas on how to improve it.
    > Perhaps someone would show what they believe to be
    > an improvement.  I realized after thinking about
    > your earlier comments that the member variable
    > partialFlush could be a static local variable.
    >
    > bool
    > SendBuffer::Flush ()
    > {
    >   uint32_t const all = buf_.size();
    >   uint32_t numWritten = 0;
    >   static bool partialFlush = false;
    >   if (partialFlush) {
    >     ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > segment =
    >                segmented_iterator<unsigned char,
    > chunk_size>(buf_.begin()).segment();
    >
    >     numWritten = Write(sock_, &buf_[0], segment.size());
    >     if (numWritten < segment.size()) goto ending;
    >     partialFlush = false;
    >   }
    >
    >   {
    >     int32_t const chunks = (all - numWritten) / chunk_size;
    >     for (int32_t ii = 0; ii < chunks; ++ii) {
    >       uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
    >       numWritten += bytes;
    >       if (bytes < chunk_size) goto ending;
    >     }
    >   }
    >
    >   if (numWritten < all) {
    >     numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
    >   }
    >
    > ending:
    >   buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    >   if (numWritten == all) return true;
    >
    >   partialFlush = true;
    >   return false;
    >
    >
    >
    >
    >
    >
    >
    > }



    How would you rate this function -- with a 1 being terrible
    and a 10 being excellent?
     
    Ebenezer, Sep 18, 2011
    #9
  10. Ebenezer

    Ian Collins Guest

    On 09/19/11 08:30 AM, Ebenezer wrote:
    > On Sep 15, 7:23 pm, Ebenezer<> wrote:
    >>
    >> I started by asking for ideas on how to improve it.
    >> Perhaps someone would show what they believe to be
    >> an improvement. I realized after thinking about
    >> your earlier comments that the member variable
    >> partialFlush could be a static local variable.
    >>
    >> bool
    >> SendBuffer::Flush ()
    >> {
    >> uint32_t const all = buf_.size();
    >> uint32_t numWritten = 0;
    >> static bool partialFlush = false;
    >> if (partialFlush) {
    >> ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    >> segment =
    >> segmented_iterator<unsigned char,
    >> chunk_size>(buf_.begin()).segment();
    >>
    >> numWritten = Write(sock_,&buf_[0], segment.size());
    >> if (numWritten< segment.size()) goto ending;
    >> partialFlush = false;
    >> }
    >>
    >> {
    >> int32_t const chunks = (all - numWritten) / chunk_size;
    >> for (int32_t ii = 0; ii< chunks; ++ii) {
    >> uint32_t bytes = Write(sock_,&buf_[numWritten], chunk_size);
    >> numWritten += bytes;
    >> if (bytes< chunk_size) goto ending;
    >> }
    >> }
    >>
    >> if (numWritten< all) {
    >> numWritten += Write(sock_,&buf_[numWritten], all - numWritten);
    >> }
    >>
    >> ending:
    >> buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    >> if (numWritten == all) return true;
    >>
    >> partialFlush = true;
    >> return false;
    >> }

    >
    >
    > How would you rate this function -- with a 1 being terrible
    > and a 10 being excellent?


    You don't appear to have headed any of the suggestions.

    At review, I'd still give it a 1 because it isn't clear what it does,
    why it uses fixed width types and it still contains gratuitous gotos.

    --
    Ian Collins
     
    Ian Collins, Sep 18, 2011
    #10
  11. Ebenezer

    Ebenezer Guest

    On Sep 18, 4:34 pm, Ian Collins <> wrote:
    > On 09/19/11 08:30 AM, Ebenezer wrote:
    >
    >
    >
    >
    >
    >
    >
    >
    >
    > > On Sep 15, 7:23 pm, Ebenezer<>  wrote:

    >
    > >> I started by asking for ideas on how to improve it.
    > >> Perhaps someone would show what they believe to be
    > >> an improvement.  I realized after thinking about
    > >> your earlier comments that the member variable
    > >> partialFlush could be a static local variable.

    >
    > >> bool
    > >> SendBuffer::Flush ()
    > >> {
    > >>    uint32_t const all = buf_.size();
    > >>    uint32_t numWritten = 0;
    > >>    static bool partialFlush = false;
    > >>    if (partialFlush) {
    > >>      ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > >> segment =
    > >>                 segmented_iterator<unsigned char,
    > >> chunk_size>(buf_.begin()).segment();

    >
    > >>      numWritten = Write(sock_,&buf_[0], segment.size());
    > >>      if (numWritten<  segment.size()) goto ending;
    > >>      partialFlush = false;
    > >>    }

    >
    > >>    {
    > >>      int32_t const chunks = (all - numWritten) / chunk_size;
    > >>      for (int32_t ii = 0; ii<  chunks; ++ii) {
    > >>        uint32_t bytes = Write(sock_,&buf_[numWritten], chunk_size);
    > >>        numWritten += bytes;
    > >>        if (bytes<  chunk_size) goto ending;
    > >>      }
    > >>    }

    >
    > >>    if (numWritten<  all) {
    > >>      numWritten += Write(sock_,&buf_[numWritten], all - numWritten);
    > >>    }

    >
    > >> ending:
    > >>    buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    > >>    if (numWritten == all) return true;

    >
    > >>    partialFlush = true;
    > >>    return false;
    > >> }

    >
    > > How would you rate this function -- with a 1 being terrible
    > > and a 10 being excellent?

    >
    > You don't appear to have headed any of the suggestions.


    I'm not opposed to the size_t suggestion, but am wanting to
    focus on the bigger picture.

    >
    > At review, I'd still give it a 1 because it isn't clear what it does,
    > why it uses fixed width types and it still contains gratuitous gotos.
    >


    All of the code is free -- gratuitous. Meanwhile no one has
    provided an alternative they think is better.
     
    Ebenezer, Sep 19, 2011
    #11
  12. Ebenezer

    Geoff Guest

    On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer
    <> wrote:

    >All of the code is free -- gratuitous. Meanwhile no one has
    >provided an alternative they think is better.


    Gratuitous in this context means unnecessary. Not gratis. But you knew
    that.

    Refactor your function, get rid of the goto's. They are bad practice.

    Don't worry about writing redundant lines in your functions, don't try
    to optimize them away with goto's as you have done in your function,
    let the compiler optimize them away, it's much better at it than you
    are. What you should be writing for is clarity and maintainability.
     
    Geoff, Sep 19, 2011
    #12
  13. Ebenezer

    Ebenezer Guest

    On Sep 18, 9:08 pm, Geoff <> wrote:
    > On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer
    >
    > <> wrote:
    > >All of the code is free -- gratuitous.  Meanwhile no one has
    > >provided an alternative they think is better.

    >
    > Gratuitous in this context means unnecessary. Not gratis. But you knew
    > that.
    >
    > Refactor your function, get rid of the goto's. They are bad practice.
    >
    > Don't worry about writing redundant lines in your functions, don't try
    > to optimize them away with goto's as you have done in your function,
    > let the compiler optimize them away, it's much better at it than you
    > are. What you should be writing for is clarity and maintainability.


    Still no code.
     
    Ebenezer, Sep 19, 2011
    #13
  14. Ebenezer

    Geoff Guest

    On Sun, 18 Sep 2011 19:46:06 -0700 (PDT), Ebenezer
    <> wrote:

    >On Sep 18, 9:08 pm, Geoff <> wrote:
    >> On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer
    >>
    >> <> wrote:
    >> >All of the code is free -- gratuitous.  Meanwhile no one has
    >> >provided an alternative they think is better.

    >>
    >> Gratuitous in this context means unnecessary. Not gratis. But you knew
    >> that.
    >>
    >> Refactor your function, get rid of the goto's. They are bad practice.
    >>
    >> Don't worry about writing redundant lines in your functions, don't try
    >> to optimize them away with goto's as you have done in your function,
    >> let the compiler optimize them away, it's much better at it than you
    >> are. What you should be writing for is clarity and maintainability.

    >
    >Still no code.


    Ah! So it's not about what do you think of this function but it's
    about fix this function if you can. We gave you our thoughts on this
    function. We are not obligated to fix it for you.
     
    Geoff, Sep 19, 2011
    #14
  15. Ebenezer

    Miles Bader Guest

    Geoff <> writes:
    > Refactor your function, get rid of the goto's. They are bad practice.


    But let's be fair -- while goto is often[*] used badly, and in such
    cases makes code harder to understand/maintain, it isn't somehow the
    kiss of death. There are reasonable uses of goto.

    So although the presence of "goto" in code is a big warning sign
    (especially in C++, where RIAA tends to supplant "goto for error
    cleanup" idioms), is seems unreasonable to thus grade code a "1"
    without actually looking at the details of how its used.

    [*] maybe less so these days than in the past, as it's acquired such a
    huge negative reputation that non-clueful programmers are likely to
    avoid it entirely on that basis.

    -Miles

    --
    Learning, n. The kind of ignorance distinguishing the studious.
     
    Miles Bader, Sep 19, 2011
    #15
  16. Ebenezer

    Ebenezer Guest

    On Sep 18, 10:12 pm, Geoff <> wrote:
    > On Sun, 18 Sep 2011 19:46:06 -0700 (PDT), Ebenezer
    >
    >
    >
    >
    >
    > <> wrote:
    > >On Sep 18, 9:08 pm, Geoff <> wrote:
    > >> On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer

    >
    > >> <> wrote:
    > >> >All of the code is free -- gratuitous. Meanwhile no one has
    > >> >provided an alternative they think is better.

    >
    > >> Gratuitous in this context means unnecessary. Not gratis. But you knew
    > >> that.

    >
    > >> Refactor your function, get rid of the goto's. They are bad practice.

    >
    > >> Don't worry about writing redundant lines in your functions, don't try
    > >> to optimize them away with goto's as you have done in your function,
    > >> let the compiler optimize them away, it's much better at it than you
    > >> are. What you should be writing for is clarity and maintainability.

    >
    > >Still no code.

    >
    > Ah! So it's not about what do you think of this function but it's
    > about fix this function if you can. We gave you our thoughts on this
    > function. We are not obligated to fix it for you.


    I have said numerous times I'm looking for an alternative
    that is thought to be better.
     
    Ebenezer, Sep 19, 2011
    #16
  17. Ebenezer

    Ian Collins Guest

    On 09/19/11 01:25 PM, Ebenezer wrote:
    > On Sep 18, 4:34 pm, Ian Collins<> wrote:
    >> On 09/19/11 08:30 AM, Ebenezer wrote:
    >>

    Why does google put in these huge gaps? I didn't include it.
    >>
    >>> On Sep 15, 7:23 pm, Ebenezer<> wrote:

    >>
    >>>> ending:
    >>>> buf_.erase(buf_.begin(), buf_.begin() + numWritten);
    >>>> if (numWritten == all) return true;

    >>
    >>>> partialFlush = true;
    >>>> return false;
    >>>> }

    >>
    >>> How would you rate this function -- with a 1 being terrible
    >>> and a 10 being excellent?

    >>
    >> You don't appear to have headed any of the suggestions.

    >
    > I'm not opposed to the size_t suggestion, but am wanting to
    > focus on the bigger picture.
    >
    >> At review, I'd still give it a 1 because it isn't clear what it does,
    >> why it uses fixed width types and it still contains gratuitous gotos.

    >
    > All of the code is free -- gratuitous. Meanwhile no one has
    > provided an alternative they think is better.


    Everything at after your ending label could be in a bool returning
    member function.

    --
    Ian Collins
     
    Ian Collins, Sep 19, 2011
    #17
  18. Ebenezer

    Ian Collins Guest

    On 09/19/11 04:26 PM, Miles Bader wrote:
    > Geoff<> writes:
    >> Refactor your function, get rid of the goto's. They are bad practice.

    >
    > But let's be fair -- while goto is often[*] used badly, and in such
    > cases makes code harder to understand/maintain, it isn't somehow the
    > kiss of death. There are reasonable uses of goto.
    >
    > So although the presence of "goto" in code is a big warning sign
    > (especially in C++, where RIAA tends to supplant "goto for error
    > cleanup" idioms), is seems unreasonable to thus grade code a "1"
    > without actually looking at the details of how its used.


    Every coding standard I've written or had to follow has prohibited goto
    except in exceptional (and well commented) cases.

    --
    Ian Collins
     
    Ian Collins, Sep 19, 2011
    #18
  19. Ebenezer

    Miles Bader Guest

    Ian Collins <> writes:
    >> But let's be fair -- while goto is often[*] used badly, and in such
    >> cases makes code harder to understand/maintain, it isn't somehow the
    >> kiss of death. There are reasonable uses of goto.
    >>
    >> So although the presence of "goto" in code is a big warning sign
    >> (especially in C++, where RIAA tends to supplant "goto for error
    >> cleanup" idioms), is seems unreasonable to thus grade code a "1"
    >> without actually looking at the details of how its used.

    >
    > Every coding standard I've written or had to follow has prohibited
    > goto except in exceptional (and well commented) cases.


    That isn't inconsistent with what I said.

    It isn't unreasonable for a coding standard, especially those targeted
    at non-expert programmers, to outlaw goto with documented exceptions
    like that -- the entire point of a coding standard, after all, is to
    try and model subjective judgement with something simpler, more
    consistent (within the organization), and easier to apply; this is
    very valuable when many programmers of varying skill-levels are
    working together.

    But they aren't an end in themselves, and are not, in the end,
    some sort of authoritative statement.

    So if the OP was saying "goto gets your code grade 1 _in our
    organization, because it violates our coding standards_", that would
    be reasonable. But of course that isn't the scenario here.

    [Remember, I'm _not_ defending the OP's use of goto, I'm simply saying
    that goto isn't _universally_ bad, and a nuanced discussion of code
    quality shouldn't use simplistic shortcuts like that, even if they're
    justified in many more practical applications.]

    -Miles

    --
    On a bad day, I see brewers still talking as though all beer were
    consumed in the pub, by the pint -- by insatiably thirsty Yorkshire
    steelworkers who have in reality long ago sought work as striptease
    artists. [Michael Jackson]
     
    Miles Bader, Sep 19, 2011
    #19
  20. Ebenezer

    hanukas Guest

    On Sep 15, 8:01 am, Ebenezer <> wrote:
    > Shalom
    >
    > How to improve this function?
    >
    > bool
    > SendBuffer::Flush ()
    > {
    >   uint32_t const all = buf_.size();
    >   uint32_t numWritten = 0;
    >   if (partialFlush_) {
    >     ::neolib::segmented_array<unsigned char, chunk_size>::segment&
    > segment =
    >                segmented_iterator<unsigned char,
    > chunk_size>(buf_.begin()).segment();
    >
    >     numWritten = Write(sock_, &buf_[0], segment.size());
    >     if (numWritten < segment.size()) goto ending;
    >     partialFlush_ = false;
    >   }


    Someone suggested that "goto" isn't optimum solution, so you could
    restructure the control flow:

    -->

    if (numWritten >= segment.size())
    {
    partialFlush_ = false;
    }
    }

    if (!partialFlush)
    {
    ...
    }

    //ending:

    The first goto would leave the "partialFlush" into state "true", so
    this control flow change would take care of that. The second one is a
    break from the for-loop which is easy to reproduce with a signal
    variable.

    What's the problem you are having with this code? You feel that it
    should be more satisfying to the craft or something more functional
    than that?
     
    hanukas, Sep 19, 2011
    #20
    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. Colin Basterfield

    XSLT + XML - design thoughts

    Colin Basterfield, Dec 16, 2003, in forum: ASP .Net
    Replies:
    0
    Views:
    416
    Colin Basterfield
    Dec 16, 2003
  2. =?Utf-8?B?RGFuIE5hc2g=?=

    dictionary concept - thoughts on how to do plz!

    =?Utf-8?B?RGFuIE5hc2g=?=, Nov 8, 2004, in forum: ASP .Net
    Replies:
    0
    Views:
    347
    =?Utf-8?B?RGFuIE5hc2g=?=
    Nov 8, 2004
  3. =?Utf-8?B?Q3JhaWc=?=

    any thoughts?

    =?Utf-8?B?Q3JhaWc=?=, Dec 20, 2005, in forum: ASP .Net
    Replies:
    0
    Views:
    364
    =?Utf-8?B?Q3JhaWc=?=
    Dec 20, 2005
  4. Rajan
    Replies:
    4
    Views:
    335
    red floyd
    May 27, 2005
  5. Rudi Cilibrasi
    Replies:
    13
    Views:
    290
    Rick DeNatale
    Jul 4, 2008
Loading...

Share This Page