Early returns

[Alf P. Steinbach]

I feel that I'm missing something, some way to write this in a more
clear way than early returns, without getting into extreme indentation:

void check_incoming()
override
{
if( Base::buffer_.pointer_to<Event>() != nullptr )
{
return; // Already buffered an event.
}

winapi::Input_record event_data = {};
winapi::Double_word n_items = 0;

// TODO: x propagation
winapi::PeekConsoleInputW( keyboard_, &event_data, 1, &n_items )
|| fail( "Event_stream::Impl::check_incoming:
PeekConsoleInputW failed" );
if( n_items == 0 )
{
return;
}

const auto kind = event_data.EventType;
if( kind == winapi::Event_kind::key )
{
const auto& ev = event_data.Event.KeyEvent;
const auto key_code = static_cast<Keycodes::Enum>(
ev.wVirtualKeyCode );
const bool is_autorepeat =(ev.bKeyDown and is_in(
pressed_keys_, key_code ));
if( not is_autorepeat )
{
if( ev.bKeyDown )
{
pressed_keys_.insert( key_code );
}
else
{
pressed_keys_.erase( key_code );
}
Base::buffer_ = Boxed_event{ Key_event{
Key{ key_code, ev.uChar.UnicodeChar },
(ev.bKeyDown? Key_event::Action::pressed :
Key_event::Action::released)
} };
return;
}
}

// Best effort to consume the ignored event, to help reduce the
// chance of Windows' event buffer becoming full.
winapi::ReadConsoleInputW( keyboard_, &event_data, 1, &n_items );
}

Suggestions?

Cheers,

- Alf

[Ian Collins]

On 07/ 4/16 08:04 PM, Alf P. Steinbach wrote:
> I feel that I'm missing something, some way to write this in a more
> clear way than early returns, without getting into extreme indentation:

It looks like early returns are the clearest solution so why worry?

If you want better clarity, throw in a few usings!

>
> void check_incoming()
> override
> {
> if( Base::buffer_.pointer_to<Event>() != nullptr )
> {
> return; // Already buffered an event.
> }

Testing preconditions this way is, at least in my opinion, much clearer
that if/else blocks.

--
Ian

[Paavo Helde]

On 4.07.2016 11:04, Alf P. Steinbach wrote:
> I feel that I'm missing something, some way to write this in a more
> clear way than early returns, without getting into extreme indentation:

What's wrong with early returns? In C++ one may get an early return
basically anywhere via exceptions, so all the code must be prepared for
early returns anyway (i.e. use RAII).

Cheers
Paavo

[Öö Tiib]

On Monday, 4 July 2016 11:06:57 UTC+3, Alf P. Steinbach wrote:
> I feel that I'm missing something, some way to write this in a more
> clear way than early returns, without getting into extreme indentation:

SESE in C++ can be achieved with 'goto's, splitting code into
more functions and/or usage of short-circuiting of inbuilt &&,
|| and ?: operators ... like in C. SESE is generally not considered
"more clear" in C++.

[K. Frank]

Hello Alf!

On Monday, July 4, 2016 at 4:06:57 AM UTC-4, Alf P. Steinbach wrote:
> I feel that I'm missing something, some way to write this in a more
> clear way than early returns, without getting into extreme indentation:

> ...
> Suggestions?

I agree with several other of your respondents that
"early returns" can be perfectly appropriate, and are
often the right tool for the job.

Öö brought up the "SESE" acronym. I understand the
motivation behind this proposed rule, but I never
thought it made sense as a "hard and fast" rule, or
even one "with rare exception."

I often find "early returns" to be the most natural
approach to validating input to a function, especially
input from an external source where you can't rely on
your upstream code to have passed on sane input. E.g.,

if (crude_input_check_fails) return;
if (number_of_fields_not_sane) return;
if (tag_1_not_recognized) return;
if (value_1_invalid) return;
// ...
do_real_work_knowing_input_is_valid();

What's not to like? To me, such code is well-organized
and clear. Both your intent and mechanism for achieving
it is clearly expressed.

A "hidden" return buried deep in some code fragment in
the bowels of the function could be more error prone,
and would probably be bad style, but don't let some
"SESE" rule force you to throw out the good-code baby
along with the bad-code bath water.

Besides, in a similar vein, some of us even use (gasp!)
exceptions for non-exceptional code paths (the horror!):

try { parse_input(); }
catch (BadInputException e) { // parse failed }

void parse_input() {
if (crude_check_fails) throw BadInputException ("crude check");
if (field_split_fails) throw BadInputException ("bad fields");
parse_field_1();
// ...
}

void parse_field_1() {
if (bad_tag1) throw BadInputException ("bad tag 1");
parse_value_1();
}

void parse_value_1() {
if (bad_value_1) throw BadInputException ("bad value 1");
// ...
}

// ...

This is like early return, except for a series of nested
function calls.

(To avoid any semantic discussions, let me state for the
record that I do not consider handling ill-formed external
input to be an exceptional code path.)

> Cheers,
>
> - Alf


Cheerio ... And Happy Hacking!


K. Frank

[guy.tr...@gmail.com]

Perhaps OT, but I did wonder about this construction:

> winapi::PeekConsoleInputW( keyboard_, &event_data, 1, &n_items )
> || fail( "Event_stream::Impl::check_incoming:PeekConsoleInputW failed" );

isn't that equivalent to:

> if( !winapi::PeekConsoleInputW( keyboard_, &event_data, 1, &n_items ) )

> {
> fail( "Event_stream::Impl::check_incoming:PeekConsoleInputW failed" );
> }

?

If so, are there any advantages to the first construction? It took me a
moment to work out what it did (perhaps because the result of the or isn't
used).

Maybe this is an idiom unfamiliar to me.

Back on topic: I don't think generally that multiple returns are a problem.
The only one that would give me pause is the return slightly buried in the
if( kind == winapi::Event_kind::key ) clause. But I think that the obvious
way to avoid it - moving that code into a new function - introduces more
complexity than it removes.

cheers - Guy

[Mr Flibble]

On 04/07/2016 09:04, Alf P. Steinbach wrote:
> I feel that I'm missing something, some way to write this in a more
> clear way than early returns, without getting into extreme indentation:

[snip]

We have been over this several times before so I am wondering if you are
trolling.

Again:

SESE is a C idiom and SEME is fine in C++ as C++ has RAII (the main
motivation for SESE in C is related to resource leaks, a problem solved
in C++ by RAII).

SEME is unavoidable in C++ thanks to exceptions and multiple return
statements are usually fine if the function is not egregiously large. If
the function is egregiously large then consider decomposing it.

/Flibble

[Daniel]

On Monday, July 4, 2016 at 3:42:09 PM UTC-4, Mr Flibble wrote:
> On 04/07/2016 09:04, Alf P. Steinbach wrote:
> > I feel that I'm missing something, some way to write this in a more
> > clear way than early returns, without getting into extreme indentation:
> [snip]
>
> We have been over this several times before so I am wondering if you are
> trolling.
>

This is why, Alf's previous comments regrading

[Christian Gollwitzer]

Am 05.07.16 um 06:31 schrieb Stefan Ram:
> r...@zedat.fu-berlin.de (Stefan Ram) writes:
>> if( hungry )
>> eat();
>> one needs to write
>> if( !hungry )return;
>> eat();
>
> And the first might be more "fluent", i.e., in English
> one says "If you are hungry, eat something!",
> not "If you are not hungry, leave! Eat something!«

It depends on the predicate. If instead of hungry() you have full(),
it's exactly the opposite:

if (!full()) eat();


if(full()) return;
eat();

You can't always influence the sign of the predicates, e.g. if it comes
out of a library.

Christian

[Öö Tiib]

On Monday, 4 July 2016 22:40:09 UTC+3, guy.tr...@gmail.com wrote:
> Perhaps OT, but I did wonder about this construction:
>
> > winapi::PeekConsoleInputW( keyboard_, &event_data, 1, &n_items )
> > || fail( "Event_stream::Impl::check_incoming:PeekConsoleInputW failed" );
>
> isn't that equivalent to:
>
> > if( !winapi::PeekConsoleInputW( keyboard_, &event_data, 1, &n_items ) )
> > {
> > fail( "Event_stream::Impl::check_incoming:PeekConsoleInputW failed" );
> > }
> ?

Yes it is.

> If so, are there any advantages to the first construction? It took me a
> moment to work out what it did (perhaps because the result of the or isn't
> used).

There are several mild advantages. For example it avoids ! that is sometimes
hard to notice and it is some characters more terse and it can be used in
places where expression is allowed but not 'if' statement. Reading it out
"peek-console-input or fail" ... does not feel too obscure.

>
> Maybe this is an idiom unfamiliar to me.

Alf likes to invent such idioms. I like this one better than his "hopefully",
that he used sort of like "hopefully peek-console-input or throw".

In general it is part of engineer's work to invent. C++ is itself a work in
and so the set of its usage idioms can't be complete.

[Alf P. Steinbach]

On 04.07.2016 19:29, Stefan Ram wrote:
> r...@zedat.fu-berlin.de (Stefan Ram) writes:
>> void check_incoming2
>> ( winapi::Input_record & event_data,
>> winapi::Double_word n_items )
>> { const auto kind = event_data.EventType;

>> if( kind == winapi::Event_kind::key )

>> { if( check_incoming3( event_data, n_items, kind ) )return; }

>> winapi::ReadConsoleInputW( keyboard_, &event_data, 1, &n_items ); }
>

> There is an early return left above, so:
>
> void check_incoming2
> ( winapi::Input_record & event_data,
> winapi::Double_word n_items )
> { const auto kind = event_data.EventType;
> if
> ( kind == winapi::Event_kind::key &&
> check_incoming3( event_data, n_items, kind ) ); else

> winapi::ReadConsoleInputW( keyboard_, &event_data, 1, &n_items ); }

I used a slightly more course-grained functional decomposition, leaving
one early return and transforming the other returns to more meaningful
value returns, so that now the abstract event stream class is like

class Event_stream
: public Non_copyable
, public Non_movable
{
private:
class Impl;

protected:
Boxed_event buffer_;

struct Check_result{ enum Enum{
event_ignored, event_accepted, no_available_event
}; };

virtual auto accept_or_ignore_incoming_if_any()
-> Check_result::Enum
= 0;

void check_incoming()
{
if( buffer_.pointer_to<Event>() != nullptr )

{
return; // Already buffered an event.
}

while( accept_or_ignore_incoming_if_any() ==
Check_result::event_ignored )
{;}
}

virtual ~Event_stream() {}
Event_stream() {}

public:
template< class Event_kind >
auto has()
-> bool
{
check_incoming();
return (buffer_.pointer_to<Event_kind>() != nullptr);
}

auto next_event()
-> Boxed_event
{
Boxed_event result{ buffer_ };

buffer_ = Boxed_event{}; // Very empty.
return result;
}

static auto singleton() // An `Impl` defined by the
system's impl.
-> Ref_<Event_stream>;
};

… and then in the Windows implementation class there is

auto accept_or_ignore_incoming_if_any()
-> Check_result::Enum
override
try
{

winapi::Input_record event_data = {};
winapi::Double_word n_items = 0;

winapi::PeekConsoleInputW( keyboard_, &event_data, 1,
&n_items )

|| fail( "PeekConsoleInputW failed" );

if( n_items == 0 )
{

return Check_result::no_available_event;

}

const auto kind = event_data.EventType;
if( kind == winapi::Event_kind::key )
{
const auto& ev = event_data.Event.KeyEvent;
const auto key_code = static_cast<Keycodes::Enum>(
ev.wVirtualKeyCode );

const bool is_autorepeat = (ev.bKeyDown and is_in(

pressed_keys_, key_code ));
if( not is_autorepeat )
{
if( ev.bKeyDown )
{
pressed_keys_.insert( key_code );
}
else
{
pressed_keys_.erase( key_code );
}
Base::buffer_ = Boxed_event{ Key_event{
Key{ key_code, ev.uChar.UnicodeChar },
(ev.bKeyDown? Key_event::Action::pressed :
Key_event::Action::released)
} };

winapi::ReadConsoleInputW( keyboard_, &event_data,
1, &n_items );

return Check_result::event_accepted;

}
}

// Best effort to consume the ignored event, to help reduce the
// chance of Windows' event buffer becoming full.
winapi::ReadConsoleInputW( keyboard_, &event_data, 1,
&n_items );

return Check_result::event_ignored;
} CPPX_RETHROW_X // Prepends the qualified function name
to the message.

… which doesn't yet deal with mouse events, but's like in the old days
one bought FM receivers “ready for stereo”: they could more or less
easily be upgraded to stereo, or at least that was intended and hoped.

I think the names of the functions are now awkward, though. :)

Cheers!, and thanks,

- Alf (ungood at naming non-simple functions)