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@xxxxxxxxxx>
> > > ---
> > > 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;
}
...
>
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
