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

[Eduardo Lima (Etrunko)]

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?


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