[Owncloud] Code review? OC_Image class

[Thomas Olsen]

Thanks for the feedback.

I've rearranged the quotes for easier reading. Maybe we should make it a 
policy to bottom-post on the mailing list?

On Sunday 01 January 2012 19:05 Bartek Przybylski wrote:
> > On Sun, Jan 1, 2012 at 18:27, Bartek Przybylski <bart.p.pl at gmail.com> > >> 
Thanks for your work for owncloud :) i hope oc_image will fast fit
> >> into current ownCloud source
> >>> Anyone willing to do a little code review and catch my errors?
> >> here are my suggestions:
> >> __constructor, __destructor: should it return any value? if yes, then
> >> you also should return true on success
> >> loadFromFile:119 -> there is no need for checking is given parameter
> >> string, if not then is_file will evaluate to false

You're right. I was being overcautious.

> >> maybe add width and height functions ?

Yeah, I totally forgot that :-D Pretty obvious methods to have.

> >> mayba add function put(resource, x, y, w, h) which will copy local
> >> resource to given resource on coordins x and y and crop it to h and w
> >> dimentions ?

I like that :-) Crop the local resource before copying it to the image or crop 
the image afterwards?

> 2012/1/1 Robin Appelman <icewind1991 at gmail.com>:
> > Hi,
> > 
> > constructors and destructors shouldn't return anything.

I was a bit unsure on that. Maybe I should have a method for telling if the 
object contains a valid image resource instead?

> > I would rename imageResource() to resource(), so you get
> > $image->resource() instead of $image->imageResource() which seems
> > redundant to me.

I agree but wont it conflict with the $resource property already there? I 
could of course just rename that one to _resource or something.

> > Also, it seems better to me to use imagecreatefrompng/jpg/etc instead
> > of imagecreatefromstring(file_get_contents()) which needs to load the
> > image in memory twice.

You're probably right. I was just being lazy ;-) I'll add some logic for that.

> >> line 246: } else { -> } \n else {
> > '}else{' is currently used in oc and as far as I know the "correct"
> > way to do it according to our guidelines.
> '}else{' is not correct if i understand guideline correct
> http://owncloud.org/contribute/development-setup/
> closing brackets in a sepearate line

I interpreted the guidelines as closing brackets on separate lines for a 
logical block as in:

if(something) {
  //
} else {
  //
} // closing bracket

I'm not sure what's correct, but that's how it's done in the code I've seen in 
OC (and my preferred style ;)
 
-- 
Med venlig hilsen / Best Regards

Thomas Olsen