[haiku-commits] Re: r40317 - haiku/trunk/src/tests/kits/net/preflet/ServicesAddOn

CC'ing kallisti5@xxxxxxxxxxx, just in case he's not already on the
commits list...


On Sat, Jan 29, 2011 at 12:59 PM, Axel Dörfler <axeld@xxxxxxxxxxxxxxxx> wrote:
> kallisti5@xxxxxxxxxxx wrote:
>> Log:
>> added *basic* ability for services addon to parse xinetd, now we can
>> list active network services in the services view
>
> Please, for the name of god, use the driver settings for this.
> When in doubt, it always makes sense to look who else is going to parse
> those (it's the net_server, btw, which also completely replaces the
> need to any inetd).
>
>> +     if (ParseInetd() == B_ERROR)
>> +             ParseXinetd();
>
> Don't test for specific errors, test against it, ie:
>        if (ParseInetd() != B_OK)
>                ...
>
>>       return new ServicesAddOn(image);
>>  }
>>
>> +
>>  // #pragma mark -
>>  ServicesAddOn::ServicesAddOn(image_id image)
>
> Also two blank lines after the #pragma.
>
>> +     BScrollView* sv = new BScrollView( "system_services_scrollview",
>> +                                             fServicesListView, 
>> B_FOLLOW_ALL_SIDES,
>> +                                             B_WILL_DRAW | B_FRAME_EVENTS, 
>> false, true);
>
> Please indent only a single tab.
>
>> +     FILE *f = fopen("/boot/common/settings/network/services", "r");
>> +     if (f) {
>> +             char line[1024], *l;
>
> Please don't use variable names like 'f' and 'l'. 'i' for an index is
> the only acceptable exception, really.
>
>> +                     if (line[0] == '#' || line[0] == '\n')
>> +                             continue;
>> +                                     // Skip commented out lines
>
> Also comments count to the multi-line rule, ie. use {} here.
>
>
>> +             return B_OK;
>> +     } else
>> +             return B_ERROR;
>
> Don't use code like this, either solve the issue where it happens, ie.:
>        FILE* file = ...;
>        if (file == NULLL)
>                return errno;
>
> Or, don't use "else" - it is completely superfluous, and just makes
> parsing a bit harder than needed.
>
> Also, use meaningful error codes when available. Since this is not for
> any user to see, it doesn't really matter, but still nothing to let
> slip in.
>
>> + *           Alexander von Gluck, kallisti5@xxxxxxxxxxx
>> + */
>> +
>> +#include "NetworkSetupAddOn.h"
>> +
>> +class ServicesAddOn : public NetworkSetupAddOn {
>
> Coding style: two blank lines for each of the ones above.
>
>> +protected:
>> +     BListView*              fServicesListView;
>> +public:
>> +                                     ServicesAddOn(image_id addon_image);
>> +     BView*          CreateView(BRect* bounds);
>> +     const char*     Name() { return "Services"; };
>> +     status_t                ParseInetd();
>> +     status_t                ParseXinetd();
>
> Coding style: proper indentation.
> Please read our style guideline again, and try to follow it.
>
> Also, I actually mention all those things so that you can actually fix
> them, not just because I have nothing better to do.
>
> Bye,
>   Axel.
>
>
>

Other related posts: