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? > >> >> 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. > >> >> 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 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. A few other comments: - Maybe you should think about another name? rajmv.. is not easy to remember. - You're formatting is confusing, for example function names. They start with lowerCamelCase, and then you go on underscore_formatting.. - 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) - If you want people to comment on code, please be so kind to remove any unused commented code. Hope this helps, Matijn -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
