On Sun, Jun 12, 2011 at 6:46 PM, francescortiz <[email protected]>wrote:
> Cal, > > Thank you for your feedback. I really appreciate it. I will be > implementing what you say as soon as I can. Everything you said was > pretty clear except the part from including a RequestContext. Where is > the RequestContext missing? > This was in relation to the template rendering comment, but without a template then there's no need for request context. > > Also, about using a template instead of embedding HTML in the code, I > don't think I will be doing that unless it comes to be necessary (or > someone demands that), in order to save some function calls, like they > do in django widgets source code (if I didn't guess wrongly why they > do it this way). > Yeah, I guess it does, but you'd only really see the performance difference if the method was getting 10k hits/sec (might be worth benchmarking it to see what the perf diff actually is, it'd be a good learning experience!) > > Your comments are great. I will be fixing the race condition this next > week, and the rest of what you comment ASAP. > > Regards, > > Francesc > > > > On 12 Juny, 19:19, "Cal Leeming [Simplicity Media Ltd]" > <[email protected]> wrote: > > Sorry, one last thing. > > > > In relation to the resp html stuff, you should have actually used a > template > > for that, loaded the variables into a request context, then rendered the > > template directly. > > > > Cal > > > > On Sun, Jun 12, 2011 at 6:18 PM, Cal Leeming [Simplicity Media Ltd] < > > > > > > > > > > > > > > > > [email protected]> wrote: > > > First, the good feedback. > > > > > Good use of exception blocks, code style, built-ins, standard libs > etc.. > > > Although I can't comment on the implementation on the end product, you > have > > > clearly made an effort to use as many of the features offered by Django > (and > > > python) as possible in this project. The fact you are using a decent > code > > > repo (imo), is also a good sign. Though you still have a long way to go > > > (it's a never ending road lol), this is exceptionally good work for > > > a beginner. > > > > > And here's the criticisms / a few things that stand out.. > > > > > Firstly: > > > > > path = path.replace("..", "") > > > path = path.replace("..", "") > > > (repeated 20 odd times) > > > > > Try replacing the above with os.path.basename() and os.path.abspath(), > I > > > think a combination of both is what you are looking for (unless I've > > > mistaken what you were doing) > > > > > Secondly, > > > > > resp = '<div style="display:inline-block; > position:relative; border:1px solid black;">' > > > resp += '<img id="image_center-'+str(COUNTER)+'" > src="'+reverse('image.views.image',args=(value.image_path,'width=150&height=150&mode=scale'+extra_parms))+'" > onclick=""/>' > > > resp += '<img id="image_center_crosshair-'+str(COUNTER)+'" > src="'+reverse('image.views.crosshair')+'" style="position:absolute; left:0; > top:0;" />' > > > > > It is *extremely* bad practise to use string concatenation (even when > > > casting in-line). You should instead be using a sprintf style: > > > > > Example: > > > > > """<div class="%s"""" % ( > > > class_name > > > ) > > > > > or > > > > > "<div class='%s'" % ( class_name ) > > > > > etc > > > > > Also, instead of using string appending (+=),you should instead append > > > them to a list, then do a string join on the list ("\n".join(resp)) > > > > > Thirdly, I'm seeing a lack of RequestContext(), you should always > include > > > it (even if you don't use it), makes life easier in the future. > > > > > Forthly, you are using a global within your code (global COUNTER). > Using > > > globals within a multi-threaded application is extremely bad, and > can/will > > > introduce undesirable race conditions. And believe me, this won't be > the > > > first or last time a race condition screws you over :) > > > > > I see you are also using os.system(cmd), I would recommend looking into > > > using popen instead. > > > > > It might also be worth revisiting the code in a few days/weeks/months, > > > looking over it, and making a list of things you dislike about it. As > ones > > > experience with any language progresses, their style changes slightly > and > > > they find better ways of doing things. The key to a great programmer, > is not > > > about making mistakes, but about recognising that you made one :) *imo > > > anyway!* > > > > > Hope this helps > > > > > Cal > > > > > On Sun, Jun 12, 2011 at 2:26 PM, francescortiz < > [email protected]>wrote: > > > > >> Hi, > > > > >> I developed a django app and I would like to know what you think about > > >> it. I am relatively new to python and django so I expect more > > >> criticism than enything else. > > > > >> Description: > > >> Django application that provides resizing and thumbnailing for images > > >> and videos with the ability to set the center of attention, heads > > >> won't get cut anymore. > > > > >> Link: > > >>https://github.com/francescortiz/image > > > > >> I am thinking of calling it "django-headcut" > > > > >> Thank you, > > > > >> Francesc > > > > >> -- > > >> You received this message because you are subscribed to the Google > Groups > > >> "Django users" group. > > >> To post to this group, send email to [email protected]. > > >> To unsubscribe from this group, send email to > > >> [email protected]. > > >> For more options, visit this group at > > >>http://groups.google.com/group/django-users?hl=en. > > -- > You received this message because you are subscribed to the Google Groups > "Django users" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/django-users?hl=en. > > -- You received this message because you are subscribed to the Google Groups "Django users" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/django-users?hl=en.
