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

[Christophe Fergeau]

On Mon, Jul 18, 2016 at 10:25:25AM -0300, Eduardo Lima (Etrunko) wrote:
> Use switch/case instead of lots of conditional blocks
> 
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/ovirt-foreign-menu.c | 41 +++--
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 33ff4f1..80e40a1 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -312,51 +312,40 @@ 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++;
> -
> -if (current_state == STATE_API) {
> +switch (++current_state) {
> +case STATE_API:
>  if (menu->priv->api == NULL) {
>  ovirt_foreign_menu_fetch_api_async(menu);
> -} else {
> -current_state++;
> +break;
>  }
> -}
> -
> -if (current_state == STATE_VM) {
> +case STATE_VM:
>  if (menu->priv->vm == NULL) {
>  ovirt_foreign_menu_fetch_vm_async(menu);
> -} else {
> -current_state++;
> +break;
>  }

I'd add some comment every time we fall through to the next state,
otherwise it can be confusing as there is a break; statement, but in a
conditional.

Christophe


signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list

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

[Pavel Grunt]

On Mon, 2016-07-18 at 10:25 -0300, Eduardo Lima (Etrunko) wrote:
> Use switch/case instead of lots of conditional blocks
> 
> Signed-off-by: Eduardo Lima (Etrunko) 
Acked-by: Pavel Grunt 
> ---
>  src/ovirt-foreign-menu.c | 41 +++--
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 33ff4f1..80e40a1 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -312,51 +312,40 @@ 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++;
> -
> -if (current_state == STATE_API) {
> +switch (++current_state) {
> +case STATE_API:
>  if (menu->priv->api == NULL) {
>  ovirt_foreign_menu_fetch_api_async(menu);
> -} else {
> -current_state++;
> +break;
>  }
> -}
> -
> -if (current_state == STATE_VM) {
> +case STATE_VM:
>  if (menu->priv->vm == NULL) {
>  ovirt_foreign_menu_fetch_vm_async(menu);
> -} else {
> -current_state++;
> +break;
>  }
> -}
> -
> -if (current_state == STATE_STORAGE_DOMAIN) {
> +case STATE_STORAGE_DOMAIN:
>  if (menu->priv->files == NULL) {
>  ovirt_foreign_menu_fetch_storage_domain_async(menu);
> -} else {
> -current_state++;
> +break;
>  }
> -}
> -
> -if (current_state == STATE_VM_CDROM) {
> +case STATE_VM_CDROM:
>  if (menu->priv->cdrom == NULL) {
>  ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
> -} else {
> -current_state++;
> +break;
>  }
> -}
> -
> -if (current_state == STATE_CDROM_FILE) {
> +case STATE_CDROM_FILE:
>  ovirt_foreign_menu_refresh_cdrom_file_async(menu);
> -}
> -
> -if (current_state == STATE_ISOS) {
> +break;
> +case STATE_ISOS:
>  g_warn_if_fail(menu->priv->api != NULL);
>  g_warn_if_fail(menu->priv->vm != NULL);
>  g_warn_if_fail(menu->priv->files != NULL);
>  g_warn_if_fail(menu->priv->cdrom != NULL);
>  
>  ovirt_foreign_menu_refresh_iso_list(menu);
> +break;
> +default:
> +g_warn_if_reached();
>  }
>  }
>  

___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list

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

[Eduardo Lima (Etrunko)]

Use switch/case instead of lots of conditional blocks

Signed-off-by: Eduardo Lima (Etrunko) 
---
 src/ovirt-foreign-menu.c | 41 +++--
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
index 33ff4f1..80e40a1 100644
--- a/src/ovirt-foreign-menu.c
+++ b/src/ovirt-foreign-menu.c
@@ -312,51 +312,40 @@ 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++;
-
-if (current_state == STATE_API) {
+switch (++current_state) {
+case STATE_API:
 if (menu->priv->api == NULL) {
 ovirt_foreign_menu_fetch_api_async(menu);
-} else {
-current_state++;
+break;
 }
-}
-
-if (current_state == STATE_VM) {
+case STATE_VM:
 if (menu->priv->vm == NULL) {
 ovirt_foreign_menu_fetch_vm_async(menu);
-} else {
-current_state++;
+break;
 }
-}
-
-if (current_state == STATE_STORAGE_DOMAIN) {
+case STATE_STORAGE_DOMAIN:
 if (menu->priv->files == NULL) {
 ovirt_foreign_menu_fetch_storage_domain_async(menu);
-} else {
-current_state++;
+break;
 }
-}
-
-if (current_state == STATE_VM_CDROM) {
+case STATE_VM_CDROM:
 if (menu->priv->cdrom == NULL) {
 ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
-} else {
-current_state++;
+break;
 }
-}
-
-if (current_state == STATE_CDROM_FILE) {
+case STATE_CDROM_FILE:
 ovirt_foreign_menu_refresh_cdrom_file_async(menu);
-}
-
-if (current_state == STATE_ISOS) {
+break;
+case STATE_ISOS:
 g_warn_if_fail(menu->priv->api != NULL);
 g_warn_if_fail(menu->priv->vm != NULL);
 g_warn_if_fail(menu->priv->files != NULL);
 g_warn_if_fail(menu->priv->cdrom != NULL);
 
 ovirt_foreign_menu_refresh_iso_list(menu);
+break;
+default:
+g_warn_if_reached();
 }
 }
 
-- 
2.7.4

___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list