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

[Eduardo Lima (Etrunko)]

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


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com