Hello Tobias,
thanks for the improved patch.
Dne 4. dubna 2012 14:12 Tobias Börtitz <[email protected]> napsal(a):
> Hello Vojta,
>
> thanks for the critics. I tried to correct all the mistakes I made and
> hereby submitting the corrected patch.
>
> 2012/4/4 Vojtech Horky <[email protected]>
>>
>> Hello Tobias,
>> thanks for the patch!
>>
>> Please, use tabs for indentation. The policy is to use tab for
>> indentation and spaces for alignment (e.g. 4 spaces for command that
>> did not fit onto single line).
>
> Sorry for that. I messed up my .vimrc recently but fixed it so it is doing
> the right indentation by itself now.
> I fixed all the wrong indentations made by me, too.
Great. Thank you.
>> Thus the proper solution is to split it into more functions to make it
>> more readable. Side effect is decreasing number of indentation levels.
>> For example, it would be useful to create a function
>> bool get_yes_no_from_user(const char *message, bool default)
>> for reading yes/no and get rid of that while loop that is a lot about
>> checking for proper input and little about real action.
>
> So I created the function get_user_decision(...) as you suggested for
> getting the users input.
I think that the function could be broken down even more but I admit I
haven't look all that closely.
I still have a few comments, though.
I liked about your first implementation that it printed the default
choice (i.e. when user just pressed enter). Seems that the refactored
version misses that. It is not that big deal, but it was nice :-).
There is a possible buffer overflow - the message buffer is 60 chars
long but 41+str_size(dest_path) can easily overrun this. You could
either use asprintf, check the computed size or extend the
get_user_decision() to behave like printf function (i.e. change the
signature to (bool, const char *, ...)).
printf(message) is unsafe - consider filename with % in it. You shall
use printf("%s", message) instead.
And several minor issues.
* we usuallly write while (true) instead of for(;;)
* there is no need to put break after return in switch
I will merge this patch later this week (unless something unexpected
happens), correcting these little problems myself.
Thanks again.
- Vojta
>
> If there is more about the patch please let me know.
>
> Tobias
>>
>>
>> Cheers,
>> - Vojta
>>
>>
>> _______________________________________________
>> HelenOS-devel mailing list
>> [email protected]
>> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
>
>
>
> _______________________________________________
> HelenOS-devel mailing list
> [email protected]
> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
>
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
