Contorted logic doing a thing then undoing it, I can't identify the code smell

[Alf P. Steinbach]

The following piece of code smells ungood to me, even though I wrote it
myself:

switch( buffer[n_chars-1] )
{
case Syschar( Ascii::lf ):
--n_chars; // LF.
if( n_chars > 0 ) { --n_chars; } // CR.
buffer[n_chars++] = Syschar( '\n' );
break;
case Syschar( Ascii::cr ):
--n_chars;
break;
default: {}
}

I probably need much more coffee in order to clean that up, but then I
wouldn't sleep tonight. So, asking.

Full context:

inline auto read( In_<Array_part_ref_<Syschar>> buffer )
-> Size
{
CPPX_ASSERT( n_items( buffer ) > 0 );
Size n_chars = [&]() -> Size
{
namespace raw_winapi = winapi::raw;

raw_winapi::Doubleword unsigned_n_chars = 0;
raw_winapi::ReadConsoleW(
raw_winapi::handle_of<raw_winapi::Stream_id::std_input>(),
cast::as_ptr_to<wchar_t>( p_first( buffer ) ),
n_items( buffer ),
&unsigned_n_chars
) || fail( "cppx::winapi::console::read_line:
ReadConsoleW failed" );
return unsigned_n_chars;
}();

if( n_chars == 0 )
{
return 0; // End of file. Should not happen with
console i/o.
}

switch( buffer[n_chars-1] )
{
case Syschar( Ascii::lf ):
--n_chars; // LF.
if( n_chars > 0 ) { --n_chars; } // CR.
buffer[n_chars++] = Syschar( '\n' );
break;
case Syschar( Ascii::cr ):
--n_chars;
break;
default: {}
}

return n_chars;
}

Cheers,

- Alf

[Marcel Mueller]

On 19.01.16 20.34, Alf P. Steinbach wrote:
> The following piece of code smells ungood to me, even though I wrote it
> myself:
>
> switch( buffer[n_chars-1] )
> {
> case Syschar( Ascii::lf ):
> --n_chars; // LF.
> if( n_chars > 0 ) { --n_chars; } // CR.
> buffer[n_chars++] = Syschar( '\n' );
> break;
> case Syschar( Ascii::cr ):
> --n_chars;
> break;
> default: {}
> }

Will not work with old MAC format (CR only).

> I probably need much more coffee in order to clean that up, but then I
> wouldn't sleep tonight. So, asking.

So what is your question?

> return 0; // End of file. Should not happen with
> console i/o.

EOF might not be that uncommon with console i/o if i/o redirection takes
place.


Marcel

[Rosario19]

On Tue, 19 Jan 2016 20:34:49 +0100, "Alf P. Steinbach" wrote:

>The following piece of code smells ungood to me, even though I wrote it
>myself:
>
> switch( buffer[n_chars-1] )

so are you sure that n_chars>0

> {
> case Syschar( Ascii::lf ):

what about malformated text that has 3 lf only without CR

[Lőrinczy Zsigmond]

If I were you, I'd simply write this:

if (n_chars>0 && buffer[n_chars-1]=='\n') buffer[--n_chars]= '\0';
if (n_chars>0 && buffer[n_chars-1]=='\r') buffer[--n_chars]= '\0';

I know it is dissappontingly simple...
(Also it doesn't handle the \n\r situation)

[Alf P. Steinbach]

On 1/19/2016 9:52 PM, Marcel Mueller wrote:
> On 19.01.16 20.34, Alf P. Steinbach wrote:
>> The following piece of code smells ungood to me, even though I wrote it
>> myself:
>>
>> switch( buffer[n_chars-1] )
>> {
>> case Syschar( Ascii::lf ):
>> --n_chars; // LF.
>> if( n_chars > 0 ) { --n_chars; } // CR.
>> buffer[n_chars++] = Syschar( '\n' );
>> break;
>> case Syschar( Ascii::cr ):
>> --n_chars;
>> break;
>> default: {}
>> }
>
> Will not work with old MAC format (CR only).

Right. As shown by the context code, this is Windows direct console i/o.

>> I probably need much more coffee in order to clean that up, but then I
>> wouldn't sleep tonight. So, asking.
>
> So what is your question?

I was thinking that decrementing n_chars and then incrementing it is a
smell of imperfect logic...

>> return 0; // End of file. Should not happen with
>> console i/o.
>
> EOF might not be that uncommon with console i/o if i/o redirection takes
> place.

The direct console streams in Windows can't be easily redirected, it's
similar to reading /dev/tty in Unixland (if I recall correctly from
umpteen decades back).

In the early DOS days there was support for using an external serial
terminal instead of the monitor, but I don't think that was brought
forward into Windows.


Cheers, & thanks,

- Alf

[Alf P. Steinbach]

On 1/20/2016 6:56 AM, Rosario19 wrote:
> On Tue, 19 Jan 2016 20:34:49 +0100, "Alf P. Steinbach" wrote:
>
>> The following piece of code smells ungood to me, even though I wrote it
>> myself:
>>
>> switch( buffer[n_chars-1] )
>
> so are you sure that n_chars>0

Yes, from the

if( n_chars == 0 )
{
return 0; // End of file. Should not happen with
console i/o.
}

right before.

[Alf P. Steinbach]

Well since I didn't much explain the code you misunderstood it. :( Mea
culpa. Sorry.

This is Windows code that always gets \r\n up from the API level. The
separate testing for \r was not to handle out-of-order \n\r, but because
it can happen that the buffer doesn't have room for the ordinary \r\n's
final \n, in which case that single value is delivered via the next
read. Whichever of the two situations happen, the received control chars
are removed, and a single \n is added at the end of a complete line --
or at least that was the intention, and it seems to work (I use a very
small buffer, just 6 values, to test it). :)

The idea of nulling is good in general but for this code it's necessary
to keep explicit track of the length to avoid checking the length again,
and nulling isn't simpler than decrementing, is it?


Cheers, and thanks,

- Alf

PS: This brings up the question of how to unit-test such code
automatically, which seems to boil down to how to mock the API read
function (in this case Windows' ReadConsoleW) in a good way, maintaining
the maintainability of the code, so to speak?