How to determine code quality in PHP

[dagfinn]

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:

I see a lot of postering in this thread...most of i comes of as primadonna type of code bashing.

Instead of just bashing code ramdomly, why not explain to others reading this thread that are not "God Gifted Programmer" why the code is bad?

I tend to look for the following:

• 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.

[stereofrog]

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!

[nacho]

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.

[Ren]

Use of interfaces. (Parameter typechecking and instanceof).
Limited use of static method calls/singletons, which hinders testing/replacing.

[lastcraft]

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...

class Action {
    function __construct($session, $request) { ... }
    function do($template_repository) { ... }
    function onPageCancel() { ... }
} 

...whilst this does not...

class Action {
    function initialise() { ... }
    function control() { ... }
    function display() { ... }
    function cleanUp() { ... }
} 

It's not clear where I put my code for each of these methods. The developer did not put themselves in my shoes, as that will be my first question.

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, Marcus

[Shrike]

I think documentation should be in the mix somewhere, both Javadoc style and handwritten user guides...to sidestep this somewhat:

Having to open 10/15 different files in order to follow the course of a variable makes me really sick.

I've tried to make sense of OSCommerce community addons. 1000 lines of code without so much as a:

// This does that 

[mwmitchell]

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!

[Serberus]

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.

[agoossens]

- 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...

[Captain Proton]

Ego free code. I don't care how smart you think you are, there's someone out there smarter.

Logically speaking that is of course incorrect, given that there is a finite number of people on this planet

[McGruff]

- Good object oriented principles when necessary.
- Following from the above, not using OO when it's not necessary.

When would you say OOP is not necessary? Careful now...

[stereofrog]

Logically speaking that is of course incorrect, given that there is a finite number of people on this planet

This supposes being smart to be transitive, which of course it isn't.

[REMIYA]

It doesn't matter, if a cat is black or white, as long it catches mice .

[mwmitchell]

It doesn't matter, if a cat is black or white, as long it catches mice .

But if the cat has cancer, or a brain tumor and it dies the next day... then what?

[Young Twig]

But if the cat has cancer, or a brain tumor and it dies the next day... then what?

Then it doesn't catch mice, and then the color matters.

[kyberfabrikken]

But if the cat has cancer, or a brain tumor and it dies the next day... then what?

Then it still has 8 lives to go ...

[kobra]

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:

function __my_function() { } 

or

private function my_function() { } 

On the behalf what makes quality code here are my top 3 in the order of importance:

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?

They only have 9 lives if they invest in the extended guarantee.

[mwmitchell]

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.

[REMIYA]

Then it still has 8 lives to go ...

I like this point of view. Optimism is a must for a good PHP developer.

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!

[mmj]

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?"

[Srirangan]

More than the specifics of the code .. I'ld determine the quality of a system by its architecture ..

[Luke Redpath]

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:

// say hello to somebody
function sayHello($name) {
  echo "Hello ".$name;
} 

Pretty pointless as the function name makes it clear.

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):

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 

Now, if I run these test cases through a little agiledox script I wrote, I get the following specification:

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

And thats just a small snippet for this particular controller. Without pasting the rest of the tests code for privacy reasons, heres the rest of the agile dox generated JUST for this controller's suite of tests:

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

Now, that to me gives much more value than lots of comments scattered throughout the code, and the above specifications are runnable which means I can make sure of not only what my app does, but if its doing what it should be.

The bottom line is, comments are overrated.

[Tanus]

I really like that test class -> section, test method -> line item approach. I think the quote "It just makes sense" applies to this

[Luke Redpath]

This is the Ruby rSpec framework which gives a good overview of Behaviour Driven Development:

http://rspec.rubyforge.org/

[Buddha443556]

More than the specifics of the code .. I'ld determine the quality of a system by its architecture ..

No one wants a big ball of mud but that's what they get for ignoring software architecture.

Architecture Paradox another good read.