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] <
cal.leem...@simplicitymedialtd.co.uk> 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 <francescor...@gmail.com>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 django-users@googlegroups.com.
>> To unsubscribe from this group, send email to
>> django-users+unsubscr...@googlegroups.com.
>> 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 django-users@googlegroups.com.
To unsubscribe from this group, send email to
django-users+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-users?hl=en.
