Thoughts on function

[Ebenezer]

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

 

[Ian Collins]

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?

 

[Ebenezer]

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 .

 

[Ian Collins]

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!

 

[Ebenezer]

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

 

[Fulvio Esposito]

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

 

[Ebenezer]

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.

 

[Fulvio Esposito]

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

 

[Ebenezer]

On 09/16/11 02:34 AM, Ebenezer 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?

 

[Ian Collins]

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.

 

[Ebenezer]

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.

 

[Geoff]

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.

 

[Ebenezer]

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.

 

[Geoff]

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.

 

[Miles Bader]

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

 

[Ebenezer]

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.

 

[Ian Collins]

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


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]

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.

 

[Miles Bader]

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

 

[hanukas]

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?