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 <[email protected]> > >> 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 <[email protected]>: > > 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 _______________________________________________ Owncloud mailing list [email protected] https://mail.kde.org/mailman/listinfo/owncloud
