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