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

[Pavel Grunt]

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

>