[Devel] General handling of special cases if perferred

[Igor M Podlesny]

On 29 April 2013 12:43, Kir Kolyshkin <kir at openvz.org> wrote:
> On 04/28/2013 08:31 PM, Igor Podlesny wrote:
>>
>> ---
>>   src/lib/util.c |   19 +++++++++----------
>>   1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/lib/util.c b/src/lib/util.c
>> index 8cc066b..420a574 100644
>> --- a/src/lib/util.c
>> +++ b/src/lib/util.c
>> @@ -46,18 +46,17 @@ static const char *unescapestr(char *const src)
>>         char *p1, *p2;
>>         int fl = 0;
>>   -     if (src == NULL)
>> -               return NULL;
>
>
> I don't like this approach.
>
> In current version, a reviewer don't have to read the whole function
> for the case when src is NULL, and it's clear we don't do anything in that
> case.

   Well, another good habit is to keep functions as short and
observable as possible, so actually it doesn't matter. Lots of returns
(exit points) are much worse and redundant, bringing in special
handling for special cases.

> In your version, he have to read it in it's entirety, and it's a bit less
> clear that we are
> returning NULL for src == NULL case, plus it's an extra indentation level.
>
> For the rest of the patches concerning unescapestr() -- do you really fix
> anything
> or it's just refactoring? If it's a pure refactoring, there should be a good
> reason for it,
> which I missed from your comments.

   Well, if you take a look at side-to-side diff, you'll see, it's cleaner now:

https://bitbucket.org/poige/vzctl/diff/src/lib/util.c?diff1=2f6a93272a68b67c8f235625ad6619f078e6fe98&diff2=6c0541721fcc&at=master

>> -
>> -       for (p1 = p2 = src; *p2; ++p2) {
>> -               if (*p2 == '\\' && !fl) {
>> -                       fl = 1;
>> -               } else {
>> -                       *p1++ = *p2;
>> -                       fl = 0;
>> +       if (src != NULL) {
>> +               for (p1 = p2 = src; *p2; ++p2) {
>> +                       if (*p2 == '\\' && !fl) {
>> +                               fl = 1;
>> +                       } else {
>> +                               *p1++ = *p2;
>> +                               fl = 0;
>> +                       }
>>                 }
>> +               *p1 = 0;
>>         }
>> -       *p1 = 0;
>>         return src;
>>   }
>
>
> _______________________________________________
> Devel mailing list
> Devel at openvz.org
> https://lists.openvz.org/mailman/listinfo/devel