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?
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). 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.
