Last modified: 2011-03-13 18:06:09 UTC
Hello, i am a PHP developer (since about 2004) and working as a softwaredeveloper. I want to help mediawiki (especially the readability of the source) getting better. But first off all here are some suggestions (what i would rewrite first): * The coding conventions are very "holey" (maybe take a look at framework.zend.com which are much better) * If the Classnames, Filenames, ... would have a good convention -> autoloading would get easy and lines like "require_once(...)" are mostly history (performance is also better, because the class is only loaded when it's needed and not on suspicion that it's needed) * " are only used for SQL statements, because ' is faster * replace print with echo * old classes: methods are not declared as public / private and so on * one class = one file (not more than one class in a file) * use docblock for classes so all classes could get documentated in phpdoc or something like that * reformat every file with the formatting standards * a good "flow diagramm" where u see: first index.php, which includes this and this files, ... * ... I would have more, but if u like my suggestions, please contact me! I would help to clean the files from this things...
(In reply to comment #0) > Hello, > > i am a PHP developer (since about 2004) and working as a softwaredeveloper. > I want to help mediawiki (especially the readability of the source) getting > better. > > But first off all here are some suggestions (what i would rewrite first): > * The coding conventions are very "holey" (maybe take a look at > framework.zend.com which are much better) We do follow the Zend convention for some things. Other things we have our own preferences. > * If the Classnames, Filenames, ... would have a good convention -> autoloading > would get easy and lines like "require_once(...)" are mostly history > (performance is also better, because the class is only loaded when it's needed > and not on suspicion that it's needed) We already use an autoloader. > * " are only used for SQL statements, because ' is faster A common misconception. They're roughly equal for all intents and purposes. > * replace print with echo See above. > * old classes: methods are not declared as public / private and so on We clean this up as we go along. It's not safe to just start tacking private on things. > * one class = one file (not more than one class in a file) Personal choice. We follow this for the most part. Tiny classes for a single purpose can get tacked on if they're relevant though. > * use docblock for classes so all classes could get documentated in phpdoc or > something like that We do this in newer code and try to document as we go along. > * reformat every file with the formatting standards It happens when people get to it. Massive cleanups all at once lend themselves to mistakes. > * a good "flow diagramm" where u see: first index.php, which includes this and > this files, ... > * ... Would be cool on MediaWiki.org, if it's not already documented.
(In reply to comment #1) > (In reply to comment #0) > > Hello, > > > > i am a PHP developer (since about 2004) and working as a softwaredeveloper. > > I want to help mediawiki (especially the readability of the source) getting > > better. > > > > But first off all here are some suggestions (what i would rewrite first): > > * The coding conventions are very "holey" (maybe take a look at > > framework.zend.com which are much better) > > We do follow the Zend convention for some things. Other things we have our own > preferences. > > > * If the Classnames, Filenames, ... would have a good convention -> autoloading > > would get easy and lines like "require_once(...)" are mostly history > > (performance is also better, because the class is only loaded when it's needed > > and not on suspicion that it's needed) > > We already use an autoloader. Yes but this class is not very "good". Every path is WRITTEN there -> this means for every new class you need to edit this class...so the "automatic" thing is lost. > > > * " are only used for SQL statements, because ' is faster > > A common misconception. They're roughly equal for all intents and purposes. Are u sure? If u use echo "asdfsd $VAR asdf asdf " the compiler has to walk through the whole ouput string and search for variables. if u use echo 'asdfsd'. $VAR .'asdf asdf ' the compiler don't have to! --> It's not much but when u look at how much pageimpressions wikipedia has, where would be a slight performance improvement. > > > * replace print with echo > > See above. What does that todo with above? It's not recommended to use 2 functions for output, for 2 main reasons: * The readability gets lost * if where is an error in both of this function you would have to check both (very unlikely i know) * if sometime PHP has another/better output function/class you would have to change both and not only one > > > * old classes: methods are not declared as public / private and so on > > We clean this up as we go along. It's not safe to just start tacking private on > things. > > > * one class = one file (not more than one class in a file) > > Personal choice. We follow this for the most part. Tiny classes for a single > purpose can get tacked on if they're relevant though. * Then u load maybe unnecessary code with autoloading. * If u want to include a class u can't read this class in the filename (only the mainclass) > > > * use docblock for classes so all classes could get documentated in phpdoc or > > something like that > > We do this in newer code and try to document as we go along. > > > * reformat every file with the formatting standards > > It happens when people get to it. Massive cleanups all at once lend themselves > to mistakes. * If u reformat a file -> what can happen? > > > * a good "flow diagramm" where u see: first index.php, which includes this and > > this files, ... > > * ... > > Would be cool on MediaWiki.org, if it's not already documented. >
http://at.php.net/manual/de/function.print.php echo is faster: http://www.faqts.com/knowledge_base/view.phtml/aid/1/fid/40
print vs. echo and "constant" vs. 'constant' differ extremely little both in performance and in readability. I believe your autoloading suggestion is to make it so that there's a 1:1 mapping from class name to file name so we can get rid of the array in AutoLoader.php ; this could be done, but restructuring our entire codebase is not worth it (I also don't like it, personally, but that's not for here). In reply to comment #2: reformatting a file *theoretically* doesn't matter, but reviewing the change is difficult and errors can creep in.
(In reply to comment #4) > print vs. echo and "constant" vs. 'constant' differ extremely little both in > performance and in readability. It's not the readability from the "function" itself. But the readability get slightly away when using 2 function for the same. About performance: So u mean it doesn't matter? It does MATTER! EVERY speed improvement helps wikipedia! They need not so much hardware, so they need less money! > I believe your autoloading suggestion is to > make it so that there's a 1:1 mapping from class name to file name so we can > get rid of the array in AutoLoader.php ; this could be done, but restructuring > our entire codebase is not worth it (I also don't like it, personally, but > that's not for here). Why it's not worth? At the moment u mean it's a big change? But in future it a really nice change! I've done that in our project and now you can code a lot faster, and don't have to think about classes which you have to include. > In reply to comment #2: reformatting a file *theoretically* doesn't matter, but > reviewing the change is difficult and errors can creep in. I am using personally the new Zend Studio 7.1 which is based on Eclipse and to reformat a page you only have to press "Strg + Shift + F" -> and all is done. Reviewing is really hard right. But when it's done, it's done -> so in the future every PHP files looks the same which also brings a better readability.
Martin, have you ever seen a profiler? Or is all your wisdom coming from a book? Do note, '' vs "" is usually parsing issue, and is not an issue once you're executing opcode cache. You probably never tried using opcode cache? The AutoLoader change you suggest is not brilliant at all, as it would not allow us to maintain arbitrary code structure. And we like arbitrary code structure. Reformatting would break the blame feature, and I love blame feature! :) Anyway, none of your suggestions would have any impact, yet, please give more, so I can point out how you're wrong again ;-) Cheers, Domas
(In reply to comment #6) > Martin, > > have you ever seen a profiler? Or is all your wisdom coming from a book? Do you ever read a good book? About PHP Design Patterns or software architecture,.. > > Do note, '' vs "" is usually parsing issue, and is not an issue once you're > executing opcode cache. > You probably never tried using opcode cache? Can u give me a link where are more information that this is "usually parsing issue"? Maybe i use eAccelerator? But what is with mediawiki installation which don't use that? > The AutoLoader change you suggest is not brilliant at all, as it would not > allow us to maintain arbitrary code structure. And we like arbitrary code > structure. Maybe my suggestion for AutoLoader is not brilliant, but a lot better then writting over 500 lines with Paths.... > Reformatting would break the blame feature, and I love blame feature! :) Okay when this is the oppinion of u and most of wikipedia developers, i'm wrong here. Much code in mediawiki is old, hard to maintain, hard to read (for new developers), no clear structure can be found, and many more... IF ALL developers like this -> okay i don't want to do anything here. So FORGETT please my first suggestion to help with developing mediawiki. > Anyway, none of your suggestions would have any impact, yet, please give more, > so I can point out how you're wrong again ;-) Thanks: I'm at every point wrong and u're the PHP pro i hope i'll be sometime.
I've read some books, but unfortunately they've all been about internals or performance :( Anyway, my point was that one has to understand where are the slow parts before making any claims. As for parsing issue, start with http://blog.libssh2.org/index.php?/archives/92-Understanding-Opcodes.html For installations without opcode cache I will suggest installing opcode cache. We do work a bit to make mediawiki better for them (look at whole autoloader effort :), but we won't spend unreasonable resources for something what can be fixed by installing a cache. "500 lines with paths" is not necessarily bad - it compiles into nice in-memory array in opcode caches, which is really cheap to query, and doesn't cause unnecessary calls to filesystem. Anyway, we welcome contributions, but if you want to be a drama queen, feel free to find another project to contribute. I'm not PHP pro, I don't know how to write brilliantly elegant code (we have Tim for that ;-), but I prefer profiler output to opinions of people who have never really written high performance software. Cheers!
(In reply to comment #8) > Anyway, my point was that one has to understand where are the slow parts before > making any claims. > This is the key point: micro-optimization is a waste of time. It's much more productive (in terms of performance gain per man-hour spent) to profile the code and figure out where the big gains are than it is to change all instances of "foo $bar baz" to 'foo ' . $bar . ' baz' or whatever.
(In reply to comment #9) > (In reply to comment #8) > > Anyway, my point was that one has to understand where are the slow parts before > > making any claims. > > > This is the key point: micro-optimization is a waste of time. It's much more > productive (in terms of performance gain per man-hour spent) to profile the > code and figure out where the big gains are than it is to change all instances > of "foo $bar baz" to 'foo ' . $bar . ' baz' or whatever. I would do this, even if the "performance gain per man-hour spent" is low. The reasons are: * micro performance gain ( german quote: "Klein Vieh macht auch Mist" --> out of www.dict.cc it's in english "A penny saved is a penny earned." * code looks better (which is for me the design part not the front end ;-) ) * new developer can start faster, because they understand the code faster (IMO) * when someone new (like me) do something like that, they gain a lot of knowledge about the software itself in a short period of time -> because u see much parts of the code, so u know what is the current state if($interest==true) mail('keckeismartin@hotmail.com', '...', '...'); else echo 'I which u good luck for the future.';
(In reply to comment #10) > I would do this, even if the "performance gain per man-hour spent" is low. Sure, please feel free doing that on _your_own_ projects. Be aware though, that coming out of nowhere and starting teaching the developers of a mature project without caring to profile things yourself looks at least very silly. At most, combined with your language and persistence, it's indistinguishable from trolling.
Just as a note (since it hadn't been brought up yet), we document our coding conventions at http://www.mediawiki.org/wiki/Manual:Coding_conventions. It's actually kept pretty up to day and it documents how we prefer to format our code. I believe this page stresses, which we've tried to reinforce here, that code readability is everything. We do have a pretty standard way of formatting things, and it works for us. But once again, we're not going to reformat the entire source all at once, it just happens over time.
(In reply to comment #11) > coming out of nowhere and starting teaching the developers of a mature project > without caring to profile things yourself looks at least very silly. Thas was never my purpose to "teach" developers! I agree to you that mediawik is a mature project, so i though a target is also perfection in EVERY thing. I am not a guy who only want to use free things, also help to get better (even it's not a big step for the project). My first idea was to put all JS files in one, compress and so on. But then i found the "JS2" project. So where to start in a project which exists a long time best? Making a new componente is IMO not good, because there is a lot of knowledge required to make a good and stable componente. So the best way in my thoughts was to clean code, to get a feeling how things are programmed, what is already around and so on... > At most, combined with your language and persistence, it's indistinguishable from > trolling. Maybe my thoughts get across in a false way - I'm feeling sorry for that. (In reply to comment #12) > Just as a note (since it hadn't been brought up yet), we document our coding > conventions at http://www.mediawiki.org/wiki/Manual:Coding_conventions. It's > actually kept pretty up to day and it documents how we prefer to format our > code. > > I believe this page stresses, which we've tried to reinforce here, that code > readability is everything. We do have a pretty standard way of formatting > things, and it works for us. > > But once again, we're not going to reformat the entire source all at once, it > just happens over time. > Okay i understand that now. But i have to repeat that i never said that you or elseone have to do that. I would done that, but there seems to be no interest for that.
