On Mon, May 21, 2012 at 4:40 PM, Matijn Woudt <tijnema@xxxxxxxxx> wrote: > On Mon, May 21, 2012 at 2:31 PM, rene7705 <rene7705@xxxxxxxxx> wrote: >> On Mon, May 21, 2012 at 1:17 PM, Simon Schick >> <simonsimcity@xxxxxxxxxxxxxx> wrote: >>> Hi, Rene >>> >>> I took a quick look over your code ... >>> >>> I kind-of like the idea having all logging at one place, but the code is a >>> bit too messy if you ask me :) >>> If you would have put it on github and I would fork it - the first thing I'd >>> do is trying to get rid of the hm-lib and rewriting all in a bit more >>> object-oriented style, but that's just personal taste ;) >>> Specially the function rajmvServiceLog_graphs_raphael_calculateData() with a >>> code of ca. 280 lines is quite long ... >> >> 280 lines is long?! :) > > I haven't had a good look at your code yet, but let me comment on this > one anyway. > 280 lines is long. Take any big project, Linux kernel > (http://lxr.linux.no/linux), Wine (http://source.winehq.org), or an > large PHP project, for example Drupal, and you'll probably only find a > few functions that long. > >> >>> Additionally I think that setting an error-handler who's just returning >>> false is not a good way. Why not disable error-handling or write code that >>> produces no errors? >> >> returning false in an error handler means "do the default error handler plz".. > > Why are you setting the error handler then? Are you trying to override > an error handler set by other scripts? > Yea, I have an error catcher component (http://mediabeez.ws/products/logAndHandler) that's used by all my code by default, that was misbehaving during development of this analytics code, so I bypassed it for now. >> >>> >>> I think it would be good to mention that you're using the library adodb >>> (http://adodb.sourceforge.net/) and sitewide_rv (is it >>> that? http://mediabeez.ws/) or am I guessing wrong? >>> >>> You're talking about a sql-file ... has my anti-virus-program removed >>> something here? >> >> Could have, I did include it as an attachment in my OP. > > The mailing list probably removed it. You should never send > attachments to a mailing list, instead host them somewhere on the web > and link to it. I think there's a 2 attachments limit per mail sent to this list. > >> >>> >>> Please don't see that as destructive critic, but as hints what I would do if >>> I get to do with this code. >> >> I don't think your words are destructive criticism ;) >> >>> >>> Bye >>> Simon >> >> Yes, this rajmvServiceLog is tied into other components of mine that >> I've opensourced at http://mediabeez.ws >> And I agree it's not perfect yet by any standard.. >> >> hm.php is used for it's memory-efficient json encoding routines, to >> write out the results without building up a large text string with >> json_encode(). >> >> I'm not inclined to make the PHP code object oriented at this time, >> but once released I would allow anyone to OOP it. >> I will OOP the javascripts though. > > Note that OOP is not always the best way to go. Haven't taken a close > look at the source so can't comment. I agree. Some OOP adds unnecessary complexity to code.. Other OOP is sweet :) > >> >> I also don't think I'm going to host my opensourced code (including >> this analytics code) on github, I put out a .zip on >> http://mediabeez.ws instead, but you can still fork as far as I'm >> concerned. I often develop new code that updates older components in >> the package, and maintaining forks on github seems like a bit of a >> headache to me.. But I'll gladly incorporate improvements made by >> others back into my own code base, with credits of course. >> > > Putting it on github makes it easier for people to have a look at your > code before deciding on using it or not. If you're only putting it > there, you don't have to worry about forks because they don't have > anything to do with your repo, that's up to the people who fork it. > Putting it on github will increase the popularity, and will probably > also make developers want to 'upgrade' your code. It's up to you. Ok, I'll start putting it on github starting with the next release then (may be a few weeks).. > > A few other comments: > - Maybe you should think about another name? rajmv.. is not easy to remember. It's my initials, I don't want to change it, and for the js rajmv.* components you can do var easierName = rajmv; > - You're formatting is confusing, for example function names. They > start with lowerCamelCase, and then you go on underscore_formatting.. Yea, that's my coding style and I don't want to change it, sorry. > - in json_encode_*, there's probably a better way to write > if ( > $c == ' ' || > $c == '"' || > $c == "'" || > .... > you could use in_array with a predefined array for example. > - Why do you have atleast 4 large encode functions? If you really need > 4 different, then you might want to think about moving shared code to > sub functions and call them instead of duplicating so much code > (especially the large if statements look ugly being duplicated 4 > times) Ok, added to my to-do list the cleanup of the json_encode routines, and stripping them out of my "htmlMicroscope" component into a new php functions file. > - If you want people to comment on code, please be so kind to remove > any unused commented code. Ok, in the next release then. > > Hope this helps, > > Matijn It did, thanks. -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
