SitePoint Sponsor |
|
User Tag List
Results 1 to 25 of 190
-
May 4, 2006, 01:31 #1
- Join Date
- Jan 2004
- Location
- Oslo, Norway
- Posts
- 894
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
How to determine code quality in PHP
Following up on the "utter disgrace" thread, how about discussing what kind of code we want to see in open source projects. Mostly I think I know good code from bad when I see it, but my criteria might be idiosyncratic. I seem to remember selkirk posted something relevant to this a while ago, but I couldn't find anything in a quick search. Also, this is a relevant objection:
Originally Posted by CapitalWebHost
- Classes. "The more, the better" may be a stupid over-simplification, but my impression is that with existing projects, it's a pretty good indicator of quality. And up to a certain point, it is the inverse of the "large class" code smell.
- Unit tests. The few projects who actually have unit tests are clearly likely to have better code. Of course, the next criterion will be actual coverage, as opposed to assertEqual(2+2,4)
- No deep nesting of conditionals and loops. To a certain extent, this may be a matter of taste, but I would think that anything more than two levels is a negative indicator, since it's likely to be hard to unit test.
- Naming. For example, if the average length of variable names is less than 1.5, there's a problem.
This is just my personal list off the top of my head. I know there are fancier, more academic criteria as well. And there's security, which is a separate set of concerns, but obviously important.Dagfinn Reiersøl
PHP in Action / Blog / Twitter
"Making the impossible possible, the possible easy,
and the easy elegant" -- Moshe Feldenkrais
-
May 4, 2006, 02:47 #2
- Join Date
- Apr 2004
- Location
- germany
- Posts
- 4,324
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
It is a well-known truism that good code is like a good prose. Everyone can see when something is going wrong, but there is no cookbook recipe on how to make it good. A good style is mostly a matter of taste. I'd arrange things like this:
- Frugality.*) If you can get along without something, remove it.
- Conciseness. No methods longer than 10 lines, no classes with more than 10 methods.
- Uniformity. No matter what conventions you're using, be consistent.
*) yes, I found it in my dictionary!
-
May 4, 2006, 03:16 #3
- Join Date
- May 2003
- Location
- The Netherlands
- Posts
- 391
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
I'll pretty much sum up with what stereofrog is proposing. Besides that, I personally wouldn't care much if it is OOP or procedural, as long as it is well structured and thought off. Unit tests are not relevant to me but a clear separation of concerns, as dagfinn indicates, and a well applicated principle of single responsability (yes, that's also appliable to libraries) do miracles.
I also like clean code. Whether that implies a max of 10 lines per method/function or methods per class, I'll leave it open; I'm not too fond of hard requirements, but it has to be lean.
Another issue I'll take into account is security. Please, no register_globals or the like. No accessing of request variables directly and without checks. No session ids on an URL. No saving data in the DB without first CHECKING ...
And as a general rule of thumb, I don't want, as a developer, to have to spend more than 10 min. looking at source code to be able to figure out how an application functions. Having to open 10/15 different files in order to follow the course of a variable makes me really sick.
If in addition, the application is built in a fashion that allows me without too much hassle to add or remove features, you've probably got a fan.
EDIT .- Forgot to mention coding standards. Embrace them, whichever you may like the most, and please STICK to them.There’s more than one way to skin a cat.
-
May 4, 2006, 05:08 #4
Use of interfaces. (Parameter typechecking and instanceof).
Limited use of static method calls/singletons, which hinders testing/replacing.
-
May 4, 2006, 05:48 #5
- Join Date
- Apr 2003
- Location
- London
- Posts
- 2,423
- Mentioned
- 2 Post(s)
- Tagged
- 0 Thread(s)
Hi.
You can make a quick list from the Refactoring book code smells. If nothing else, it shows that the author has read the thing. I would add unit tests as well, especially for "frameworks" and libraries. It is highly likely I will want to patch the code at some point.
Even before looking at the code though, an easy install is a good indication. It shows that the developer is thinking about their users.
My biggest requirement though, is readable code. This needs breaking down further of course. I want to read a code snippet or class signature and know what it does. I want to know this without looking at the code underneath. This cuts it...
PHP Code:class Action {
function __construct($session, $request) { ... }
function do($template_repository) { ... }
function onPageCancel() { ... }
}
PHP Code:class Action {
function initialise() { ... }
function control() { ... }
function display() { ... }
function cleanUp() { ... }
}
This means...
1) Good naming. Both snippets are OK here.
2) Precise methods.
3) Explicit parameter passing. No mystery, behind the scenes, sniffing out of information.
4) I should not be required to contruct more than a handful of objects.
I don't think we can come up with more than a bunch of heuristics for this problem, but it's an important question. As is the associated question - "what makes a good programmer?".
yours, MarcusMarcus Baker
Testing: SimpleTest, Cgreen, Fakemail
Other: Phemto dependency injector
Books: PHP in Action, 97 things
-
May 4, 2006, 05:57 #6
- Join Date
- Jan 2005
- Location
- United Kingdom
- Posts
- 208
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
I think documentation should be in the mix somewhere, both Javadoc style and handwritten user guides...to sidestep this somewhat:
Originally Posted by nacho
PHP Code:// This does that
-
May 4, 2006, 09:47 #7
- Join Date
- May 2003
- Location
- virginia
- Posts
- 988
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
I actually can't stand seeing a class, where it's obvious that it was used just for the sake of being OOP. Consistency is really nice. Correctly named variables, classes, methods and functions. I want to be able to look at the code and understand what's going on. I've seen simple sites use nothing but gobs of classes/objects and it's crazy to me. Code should be obvious and free of ego. Simple and concise. Sometimes a neat way of doing things is just that... "neat". But not the best or most simple way. When I code now, I'm always thinking, "Would I want someone else to see this in a year from now? Would I and/or anyone else understand what's going on?". Also, NO extra features when they aren't needed! Comments are wonderful when they are simple and give an example. A README.txt file is also great for general notes about the application or site. Sometimes the code can look insane, but an explanation can help get you into the correct way of looking at it. Sorry for the mumble jumble!
-
May 4, 2006, 10:11 #8
- Join Date
- Oct 2005
- Location
- Herts, UK
- Posts
- 113
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
All good points. I go for comments when I have to explain why I'm doing something that cannot be conveyed from the syntax alone - business logic rules. The rest of the time I try to make my code succinct but convey what it is doing by use of properly named methods and variables.
My boss is obsessed with performance and will not exceed 8 characters for a variable name (querresu) - this drives me nuts! Coupled with his love of variable variables you can imagine how easy this stuff is to debug.
I try live by a quote from Martin Fowler's Refactoring when coding:
Any fool can write code a computer can understand. Good programmers write code humans can understand.Last edited by Serberus; May 5, 2006 at 00:20. Reason: Fixed typo.
-
May 5, 2006, 01:53 #9
- Join Date
- Mar 2004
- Location
- Adelaide, Australia
- Posts
- 124
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
- Human readable code. You won't be maintaining that code for the rest of your life, so make it easy for other people to understand.
- Ego free code. I don't care how smart you think you are, there's someone out there smarter.
- Smart use of design patterns. Use them only when necessary. Throwing them in when they're not really useful is just stupid.
- Good object oriented principles when necessary.
- Following from the above, not using OO when it's not necessary.
- Good method naming. Make them descriptive, but don't make them too long.
- Consistency in variable names, method names, CaSiNg, etc.
- Good use of comments. I don't want to drown in a sea of green...
- Avoiding as many Code Smells as you can.
When people open your code in a text editor the first thing they look at is the cleanliness and structure. If it looks like crap I bet they won't think too highly of you (I know I wouldn't). I don't really ask for much...
This space for rent.
-
May 5, 2006, 04:53 #10
- Join Date
- Oct 2001
- Posts
- 656
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Ego free code. I don't care how smart you think you are, there's someone out there smarter.I once had a problem.
I thought: "Oh, I know: I'll just use XML!"
Now I had two problems.
-
May 5, 2006, 04:55 #11
- Join Date
- Sep 2003
- Location
- Glasgow
- Posts
- 1,690
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by agoossens
-
May 5, 2006, 05:11 #12
- Join Date
- Apr 2004
- Location
- germany
- Posts
- 4,324
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Captain Proton
-
May 15, 2006, 13:07 #13
It doesn't matter, if a cat is black or white, as long it catches mice
.
-
May 15, 2006, 16:04 #14
- Join Date
- May 2003
- Location
- virginia
- Posts
- 988
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by REMIYA
-
May 15, 2006, 17:55 #15
- Join Date
- Dec 2003
- Location
- Albany, New York
- Posts
- 1,355
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by mwmitchell
-
May 15, 2006, 18:15 #16
- Join Date
- Jun 2004
- Location
- Copenhagen, Denmark
- Posts
- 6,157
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by mwmitchell
-
May 15, 2006, 18:36 #17
- Join Date
- Sep 2003
- Location
- Chicago
- Posts
- 190
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
When we talk about OOP and PHP you need to mention what version of PHP we are talking about (as we all know PHP4 is very limited when it comes to OOP). PHP5 on the other hand has a lot of features that help OOP. For example, which is better to understand when you see:
PHP Code:function __my_function() { }
PHP Code:private function my_function() { }
Inline Documentation (it could be only personal preference but I prefer inline documentation than a manual or some other document outside the code that explains methods, and classes)
Following Basic Naming Conventions (for example, start each function name with a verb, keep your variable names between 4-8 characters, etc)
Divide and conquer (don't create classes with 3 functions each 40 lines long, try to separate it logically)
But if the cat has cancer, or a brain tumor and it dies the next day... then what?
-
May 15, 2006, 18:57 #18
- Join Date
- May 2003
- Location
- virginia
- Posts
- 988
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Cats with more than one life only exist in cartoons!
I think that excellent code can be written in PHP 4 also. The number one rule for me; think about others when coding. It can save your reputation and a lot of head scratching for you and others.
-
May 15, 2006, 19:51 #19
Originally Posted by kyberfabrikken
Good PHP code practices. Hm:
1. Comment. After you open your project in several months, you start guessing, what is what. So comment thoroughly. Don't leave commenting for the end, when the application is ready.
2. Test. Test the application thoroughly before distributing it to the web.
3. Do not use databases.. Well, I see, most of the developers, are to disagree. So let's periphrase it. Use databases, only for projects where data is very sensitive, changes fast, regularly, and is large. So what is large? It's a relative idea. One may say 12 MB, 700 MB or 4 GB. According to my experience 99% of all the projects don't need (MySQL,Acess - like)databases. "Please, excuse us but the MySQL server is not working a the moment", "The maximum connections exceed the limitations of the MySQL server", etc.
And the cat sleeps, dreaming of more mice to come, for she had eaten them all!
-
May 15, 2006, 21:05 #20
- Join Date
- Feb 2001
- Location
- Melbourne Australia
- Posts
- 6,282
- Mentioned
- 1 Post(s)
- Tagged
- 0 Thread(s)
Trying to more specific and less general:
- No use of global variables
- No use of extract()
- No use of eval()
I don't ask for much. These are all things which make code hard to understand and (consequently) hard to maintain. Therefore, voodoo.
Getting back to the original problem, I think that forum software like vBulletin and phpBB would be infinitely better if they didn't use global variables, or eval() so extensively.
It doesn't matter so much that it's not OO, but I don't want to be forced to do grep after grep just trying to answer the question "where did this bloody variable come from?"[mmj] My magic jigsaw
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The Bit Depth Blog · Twitter · Contact me
Neon Javascript Framework · Jokes · Android stuff
-
May 15, 2006, 22:28 #21
More than the specifics of the code .. I'ld determine the quality of a system by its architecture ..
Online Startups Insight for new entrepreneurs
-
May 16, 2006, 01:48 #22
Interesting that many people are putting so much emphasis on commenting - I've discussed this in general development before, but surely clear, self-documenting code whose intent is made clear in both the code itself and its accompanying unit tests is better than what is often pretty pointless documentation. Too often you see comments along the lines of:
PHP Code:// say hello to somebody
function sayHello($name) {
echo "Hello ".$name;
}
Commenting is useful when trying to describe a particularly strange design decision, or a choice of algorithim or other individual cases but often lots of comments seem like a bad code smell to me - ask yourself, if I have to write a comment to make the intent clear, then surely my code could be clearer in the first place?
To take an example from a project I'm currently working on (RubyOnRails project), none of our controllers have comments except in a few places. The names of the controller actions and the code itself make the intent pretty clear, but if I want to see what the controller is doing, I can simply look at the unit tests, like these, for a controller action that displays a list of fees (its important to note that in a controller, a unit is generally a single action and not the entire controller - further more I take the Behaviour Driven Development style of test cases per context):
PHP Code:class Admin::Fees::FeesListTest < Admin::Fees::FeesBaseTest
fixtures :fees
def send_request
login_as :admin and get :list
end
def test_should_return_successful_response
assert_response :success
assert_template 'list'
end
def test_should_display_thirty_fees_at_a_time
set_number_of_fees_in_database(30) and get :list
assert_equal 30, assigns(:fees).length
assert_collection_of Fee, assigns(:fees)
assert_tag :tag => 'tbody',
:parent => { :tag => 'table',
:attributes => { :class => 'list_table' } },
:children => { :count => 30,
:only => { :tag => 'tr'} }
end
def test_should_display_fees_ordered_by_name_in_ascending_order
fee_names = assigns(:fees).collect { |f| f.feename }
assert_ordered fee_names, :ascending
assert_tag :tag => 'a',
:attributes => { :class => 'sort_asc' },
:content => 'Name'
end
def test_should_display_fee_search_form
assert_tag :tag => 'form',
:attributes => { :action => '/admin/fees/normal/search' },
:descendant => { :tag => 'h2', :content => 'Search Fees' }
end
def test_should_display_link_to_new_fee_screen
assert_tag :tag => 'a', :content => 'Add New'
end
protected
def create_example_fee
f = Fee.new
f.feename = 'Example Fee'
f.save(false)
end
def set_number_of_fees_in_database(amount)
Fee.destroy_all
amount.times { create_example_fee }
end
end
class Admin::Fees::DefaultFeesListTest < Admin::Fees::FeesListTest
def test_should_display_first_thirty_fees
assert_match Regexp.new("Displaying records <strong>1</strong> to <strong>30</strong>"), @response.body
end
def test_should_display_next_page_link_when_more_than_thirty_fees
set_number_of_fees_in_database(0) and get :list
assert_no_tag :tag => 'a', :content => 'Next page'
set_number_of_fees_in_database(31) and get :list
assert_tag :tag => 'a',
:attributes => { :class => 'next pagination_btn',
:href => /\?page=2/ },
:content => 'Next page'
end
def test_should_not_display_previous_page_link
assert_no_tag :tag => 'a', :content => 'Previous page'
end
end
Fees list should:
* return successful response
* display thirty fees at a time
* display fees ordered by name in ascending order
* display fee search form
* display link to new fee screen
Default fees list should:
* display first thirty fees
* display next page link when more than thirty fees
* not display previous page link
Second page of fees list should:
* display fees thirty one to sixty
* display next page link when more than sixty fees
* display previous page link
New fee screen should:
* return successful response
* display new fee form
* provide list of organisations for form
* provide list of feetypes with blank default for form
* provide an option to create another fee for the same organisation
* provide a link back to fee listing
New fee screen with referral should:
* decode previous request from referral string
* provide a link back to the previously viewed fee listing page
New fee screen without referral should:
* return successful response
* provide a link back to default fee listing page
New fee screen with organisation preset should:
* preselect selected organisation in dropdown list
Successful fee creation should:
* create new fee in database
* set new fee as updated by current logged in user
* create new fees location for new fee
* flash a success message
Successful fee creation with create another option set should:
* redirect back to new fee screen with organisation id preset
Successful fee creation without create another option set should:
* redirect back to fee listing page the user came from
Failed fee creation should:
* redirect back to new fee screen
* flash fee validation errors
The bottom line is, comments are overrated.
-
May 16, 2006, 02:27 #23
- Join Date
- Apr 2004
- Location
- Melbourne
- Posts
- 362
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
I really like that test class -> section, test method -> line item approach. I think the quote "It just makes sense" applies to this
-
May 16, 2006, 02:47 #24
This is the Ruby rSpec framework which gives a good overview of Behaviour Driven Development:
http://rspec.rubyforge.org/
-
May 16, 2006, 04:23 #25
- Join Date
- Apr 2004
- Location
- FL, USA
- Posts
- 87
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Srirangan
Architecture Paradox another good read.Simple fool to the 3rd include.
Bookmarks