[virt-tools-list] [PATCH virt-viewer 01/11] ovirt-foreign-menu: Rework states logic

[Pavel Grunt]

On Mon, 2016-07-18 at 12:18 -0300, Eduardo Lima (Etrunko) wrote:
> On 07/18/2016 11:58 AM, Pavel Grunt wrote:
> > 
> > On Mon, 2016-07-18 at 10:22 -0300, Eduardo Lima (Etrunko) wrote:
> > > 
> > > On 07/18/2016 06:15 AM, Pavel Grunt wrote:
> > > > 
> > > > 
> > > > Hi Eduardo,
> > > > 
> > > > On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
> > > > > 
> > > > > 
> > > > > Use switch/case instead of lots of conditional blocks
> > > > Yes, it is more readable
> > > > > 
> > > > > 
> > > > > 
> > > > > Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
> > > > > ---
> > > > >  src/ovirt-foreign-menu.c | 76 +++++++++++++++++++++++--------------
> > > > > ----
> > > > > ----
> > > > > ---
> > > > >  1 file changed, 36 insertions(+), 40 deletions(-)
> > > > > 
> > > > > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> > > > > index 33ff4f1..b0b8fec 100644
> > > > > --- a/src/ovirt-foreign-menu.c
> > > > > +++ b/src/ovirt-foreign-menu.c
> > > > > @@ -312,51 +312,47 @@
> > > > > ovirt_foreign_menu_next_async_step(OvirtForeignMenu
> > > > > *menu,
> > > > >      g_return_if_fail(current_state >= STATE_0);
> > > > >      g_return_if_fail(current_state < STATE_ISOS);
> > > > >  
> > > > > -    current_state++;
> > > > my preference is to keep the increment outside the switch statement
> > > > 
> > > > > 
> > > > > 
> > > > > -
> > > > > -    if (current_state == STATE_API) {
> > > > > -        if (menu->priv->api == NULL) {
> > > > > -            ovirt_foreign_menu_fetch_api_async(menu);
> > > > > -        } else {
> > > > > -            current_state++;
> > > > > +    switch (++current_state) {
> > > > Actually the increment is not needed at all thanks to your changes, imo
> > > > switch(current_state + 1) would be more readable
> > > 
> > > Alright, I don't mind at all.
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > +        case STATE_API: {
> > > > 'case' should have the same indentation as its 'switch'
> > > > 
> > > > Remove extra {}, no need to have the null check in the extra block
> > > > (applies
> > > > to
> > > > all cases)
> > > > 
> > > 
> > > I don't think so, the if checks are necessary for the initialization
> > > process, when we have everything initalized, it will fall straight to
> > > the STATE_ISOS case. Or maybe you are talking about something else and I
> > > misunderstood?
> > > 
> > 
> > the {} are not needed, but it is a minor:
> 
> The break statements are put inside of the conditional block on purpose.
yes, of course. I am not talking about break statements

> If the NULL check passes, the initialization is done asynchronously and
> that function will be responsible for calling back with the next step.
> If it fails, it means that it was already initialized, so we move to the
> next initialization, one at a time, until everything is set up and it
> will fall straight to the STATE_ISOS.
> 
> > 
> > ...
> >         case STATE_API:
> >             if (menu->priv->api == NULL) {
> >                 ovirt_foreign_menu_fetch_api_async(menu);
> >                 break;
> >             }
> >         case STATE_VM:
> >             if (menu->priv->vm == NULL) {
> >                 ovirt_foreign_menu_fetch_vm_async(menu);
> >                 break;
> >             }
> > ...
> > 

I tried to show it ^, but to be more explicit:

instead of
case A: {
  if () {
    break
  }
}

just
case A:
 if () {
   break
 }

But you have already changed it for v2 :)

Pavel
> > > 
> > > 
> 
>