On Sat, 20 Aug 2011 23:13:23 +0200 Quentin Gibeaux
<quentin.gibe...@insa-rouen.fr> said:
hey hey! finally i'm reviewing this. (it's been on my queue for a little while
now and i was away for 2 weeks so things got backlogged). note that in the
big bad world of open source, we don't normally go singing praises and
applauding everything you do. we do raise all hell about the bad things and say
nothing about the good. no comment == "well done". :) take this as an
educational experience from someone who has lived, eaten and breathed code for
the vast majority of their lives... :) so time to get grumpy! (with a smile) :)
first. if you are going to make tarballs.. you are using autoconf and tools...
USE THEM:
make dist
this makes a distribution tarball. like picture-0.0.1.tar.gz nice and cleanly.
no build directories and junk floating around in it. clean and neat.
make distcheck
also this ensures your distribution will work when built and everything is
shipped with it that should be).
general (this is for people new to programming or learning): FORMATTING
CONSISTENCY IS IMPORTANT! cod has to be READ by the person who writes it AND
others that follow later to maintain it. comments are goos. well named
functions and variables are even better. clear design patterns and consistency
is very important - you learn the projects pattern soon enough then you follow.
looking at just indentation and formatting i see all sorts inconsistencies.
these MATTER. it's like typing up an essay and printing it out and binding it
to hand in, vs scribbling it on the back of a random napkin pile you piked up
from mcdonalds, kfc and the local kebab stand with mustard, tomato sauce and
balsamic vinegar. people don't like reading your essay if you do that. :)
some quick examples i see instantly:
1)
char* imageLocation;
vs
Config_Item *ci
(note * comes right NEXT to the variable in 1 case, and in the other separated
by space(s). be consistent!
2)
{
E_Config_Dialog_Data *cfdata = NULL;
Config_Item *ci=NULL;
ci=cfd->data;
seriously? where did your indenting go? and then x = y then later x=y no
spaces. again - consistency!
3)
e_dialog_resizable_set(cfd->dia, 1);
vs
_fill_data(ci,cfdata);
space after , in one, no space in the other?
4)
if (!v) return NULL;
vs
if(!picture_conf) return;
not to mention one is indented, the other not. one has a space after if, one
does not.
5)
if(ci->imageLocation)
{
cfdata->imageLocation = strdup(ci->imageLocation);
}
the {} braces don't line up. please don't tell me you use word to edit code
and/or use proportional fonts? :)
i can go on. i just started looking at 1 file. you do know that editors like
vim, emacs, jed, joe and others AUTO INDENT for you, if you use them? it saves
you mountains of time. if you don't know how to use such tools, you get to do it
the slow hard way, but they help with some of the boring indenting work. learn
your tools and use them well! it's really important. it makes you productive.
even if they help, they don't do everything. build HABITS. always write code in
a specific way. when i write if i ALWAYS without thinking bash out:
if ()
then i go left arrow 1 instantly and my cursor is in the () middle ready to put
in the case. my brain doesn't think this. it's an automatic response by my
limbic system. i want to do an if.. out that comes. i want a new function i
brainlessly type:
void
f()
{
}
no thinking. i just have a stream of chars i then insert parameters into,
change the return type if needed. build such habits. they will eventually save
you 1000's of hours of work as good habits save you many bugs later.
ok - enough ranting about formatting of just 1 file. there's more. the
formatting discourages reading of the source...
in e_mod_main.h:
/* More mac/def; Define your own. What do you need ? */
#define CONN_DEVICE_ETHERNET 0
left over copy & pasting! if you are copy and pasting a lot of code from
somewhere to get you started.. at least follow its formatting as above. it
makes it readable. :) mising styles like conf_items variable naming with
timerFrequency (camelcase) doesn't look good. pick and choose. (normally just
choose the example that was set before you adding your code) :)
you e_configure_registry_item_del() an item without ever adding it.
you e_configure_registry_category_del() without
e_configure_registry_category_add() of the same category at init -> bad.
/* Cleanup our item list */
while (picture_conf->conf_items)
can be simpler and shorter with EINA_LIST_FREE() e.g.:
Config_Item *ci;
...
EINA_LIST_FREE(picture_conf->conf_items, ci)
{
/* cleanup stringshares */
if (ci->id) eina_stringshare_del(ci->id);
if (ci->imageLocation) eina_stringshare_del(ci->imageLocation);
/* keep the planet green */
E_FREE(ci);
}
much shorter :)
in _gc_init() you strcat to buf and then strcat a string (inst->conf_item->id)
that. id seems with a quick look to be sometimes passed in by you (controlled)
and sometimes not - i need to look more, but i smell buffer overflow danger
here.
why do you move the image to 00 and set layer to -999 when you then pass it off
to gadcon which ends up moving you elsewhere and violating your layer request
(as you now must be in the parent layer, not the one you chose)? this is an
evas/smart object and gadcon specific behavior here, but just note that
evas_object_move(inst->o_picture, 0, 0);
evas_object_layer_set(inst->o_picture, -999);
are useless there. just code-clutter.
curl! CURL! you are using curl directly? we have a whole ecore_con_url() thing
that glues it into the main loop nicely for you with minimal effort. another
efl thing to know. :)
not sure about your timers and using the gcd of their intervals - why dont you
just set up 1 timer per instance - have it go off whenever that instance
changes image. that's it. simple. timer passes instance as data ptr. instance
deletion deleted timer handle for that instance. much simpler than the code you
have and more modular and "object oriented" (each obj just has its own timer
and world)
images - you load inline. you don't use evas's preload mechanism. as you change
images often and plan on having biggish images... you might want to have used
this and then used the preload done callback then to show the newly loaded
image once the thread that loads it is done. :) and efl thing here.
you know with
e_gadcon_client_aspect_set(gcc, 16, 16);
e_gadcon_client_min_size_set(gcc, 16, 16);
you are saying you always want to be square (aspect ratio) with a min size of
16x16 (fair enough). :)
/* cleanup any stringshares here */
while (picture_conf->conf_items)
.. again - EINA_LIST_FREE() :)
:) :) :) otherwise - you did a decent job. i'm feeling nice today... so..
smile :) :)
> Hi everyone,
>
> Our student's project of making widgets for E17 is now over since couple of
> weeks and we've got a quite functioning one which isn't present in the E17
> project.
> Here it is (it's free as in free speech) :
> https://filex.insa-rouen.fr/get?k=ITT9f3ceMKv2NBXtJbX
>
> It provides you a widget that allows you to display an image (from a local
> filesystem or from the web), you can also refresh it a the rate you wish,
> like for monitoring stuff.
> The images downloaded from the web are stored in the /tmp folder (is that
> linuxproof only?).
>
> We'd like to include this widget in the E17 project, but we don't know the
> procedure.
>
> Anyway we'd like to get some feedbacks, so feel free to give yours
> (features, bugs, optimizing code…).
>
> Regards,
> Quentin Gibeaux & Jean Creusefond.
> ------------------------------------------------------------------------------
> Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
> user administration capabilities and model configuration. Take
> the hassle out of deploying and managing Subversion and the
> tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
> _______________________________________________
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
--
------------- Codito, ergo sum - "I code, therefore I am" --------------
The Rasterman (Carsten Haitzler) ras...@rasterman.com
------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2d-oct
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
