Ok, thank you, in next thread with code attached I will search
follow this suggestions.
N.B. I'd like know if pkg_add work.
savio
2008/11/15, Dennis Melentyev <[EMAIL PROTECTED]>:
> Hi Saverio,
>
> It is a good idea to write a GUI frontend and written code is much
> better than just an idea to write it, but I still see a long way to
> go.
>
> Please let me comment your code a little.
>
> And (I'd like to emphasize this) it is absolutely not an attempt to
> hurt you, but rather friendly general comments for future improvement
> of you coding skills. Please, do not take it personally.
>
> Current code is good for the proof-of-concept project. To make it into
> "production", you'll have to work a lot on architecture and
> components.
> I'd suggest to freeze it to the current functionality, fix every
> possible bug and start the next version of it, with following notes in
> mind.
>
> Typical problems are:
> 1. Heavy use of hardcoded magic constants.
> 2. The code is too "linear": Doing everything in main() is not the
> best practice. You have to separate logic into specific functions.
> Even if there is only one call for some function.
> This will improve code readability and let you maintain your code more
> easily in the future.
> 3. I hardly can see the reason for this construction:
> for (i = 1; i <= 4; i++) {
> switch (i) {
> case 1: page = gtk_vbox_new(TRUE, 0);
> ....
>
> This saves you nothing. Just make it four separate functions and call
> them in a sequence. Or, refactor code to have some common part and
> some conditional part within a cycle.
> Something like:
> for (i=0; i<LAST_PAGE; I++) {
> /* prepare page-specific data */
> switch(i) {
> case 1:
> page = ....;
> psgfe_add_button(BUTTON_ONE);
> psgfe_add_button(BUTTON_TWO);
> psgfe_add_button(BUTTON_THREE);
> break;
> case 2:
> page = ....;
> psgfe_add_button(BUTTON_ONE_A);
> psgfe_add_button(BUTTON_TWO_A);
> psgfe_add_button(BUTTON_THREE_A);
> break;
> ...
> default:
> /* Report a missed case and kill the program */
> assert(...);
> }
>
> /* Put page-independent code here */
> }
>
> 4. Absolutely no diagnostics. This hurts you in the first place. There
> is no way to understand what happen to the user when he reports a
> problem. Just consider -d or -v option in most programs.
>
> 5. Almost no error handling:
> fd = g_open("/usr/pkg/share/pkgsrcgfe/mirror.conf", O_RDONLY);
> if (fd < 0) perror("fd < 0");
> lseek(fd, 0, SEEK_SET);
>
> perror() does not aborts execution. Even more, it does output to
> strerr, but you will miss it in X environment most of the time.
> lseek() could fail also. As well as read(), malloc() and many other
> calls.
>
> Also, you leak a file handle each time a mirror.conf is opened. There
> are no g_close() or something like that.
>
> 6. The only comments in the code is a license block.
> Be verbose in comments. This does not affect performance, but help
> others _AND_ yourself to read and understand the code.
>
> Didn't tried to dig any deeper, but hope, this friendly
> suggestions/notes will help you for mutual benefit of the community.
>
> Thanks for the efforts and good luck in this great project!
>
> 2008/11/15 dark0s Optik <[EMAIL PROTECTED]>:
>> I modified program and I tell you test it, especially if it download
>> package
>> in pkg_add operation and if it works.
>> I have a very very slow internet connection and cannot test it fastly.
>>
>> The file mirror.conf must be located in /usr/pkg/share/pkgsrcgfe directory
>> and contains mirror where pkg_add must to connect. If you want change
>> mirror, then you must edit mirror.conf, but you write only one row without
>> '\n' character.
>>
>> I must complete yet this program:
>> 1) save button in mirror.conf window don't work, I must permits writing
>> and
>> saving mirror.conf from dialog window
>> 2) I'd like write multiple mirror in mirror.conf
>>
>> I ask you tell me any thing don't work.
>>
>> Regards,
>> savio
>>
>
>
>
> --
> Dennis Melentyev
>
--
only the paranoid will survive
