How to determine code quality in PHP

[Edman]

I agree with the comments bloat, sometimes I see some PHP software (think PEAR) that seems to have more comments in it than actual code. Who reads all that stuff? :S

[didimo]

(think PEAR) that seems to have more comments in it than actual code. Who reads all that stuff? :S

me!

some PEAR documentation is non existent or bad so people have to go digging into the code

[elias]

i do too. in php the code itself is not verbose about what types a method
can/should receive or what a method will resturn. the apidocs will tell
you. in best case you only have to look at the generated apidocs to get
an idea how to use a class.
of course its not that easy to determine what should be documented or
how should it be documented. here is a small brainstorm:

API Docs
- property  
  - @var with type
- method 
  - @param tag with datatype for every parameter
  - @return tag with datatype. explain what comes back if not self describing
  - @throws with exception type and when it is thrown
  - @pre for preconditions
  - explain complex params detailed (like config array)
  - explain complex return values detailed
- class
  - with an usage example. 
  - reference related classes (and user guide) with @see
  - explain what it is good for
- interface
  - explain what it is good for
  - reference example implementation
  - don't care about implementation (no @pre/@throws tags)
- general
  - do not use home grown apidoc syntax
  - document all public stuff
  - no redundant docs for PHP5 (forget @access/@static)

beside the apidocs you should document tricky sections with normal
comments.

[archigamer]

well writing good code may be subjective, but there is one good book on the topic.

http://www.amazon.com/gp/product/073...Fencoding=UTF8

Code Complete - its considered the bible in writing good, clean understandable code.

Personally, I love classes. It is the reason I switched from PHP to ASP.net. However, OOP is no easy step especially when PHP is very open in how you want to make your webpage. If you want to look at OOP, i would recommend learning Java to learn OOP then come back and apply it to PHP

[Baron Ragnarok]

I like to write good code.
I think of programming as an Art.
I like it when it looks, feels and reads good.
You know at first glance if code is good or not.
But good code and comments can lead to loss.

If I were you guys, I'd read this.

Some of us learned the hard way.

[lastcraft]

Hi...

For example, which is better to understand when you see:

function __my_function() { } 

or

private function my_function() { } 

An unfortunate example. I actually find the underscore version equally readable, and it's less typing. I'd have been happy if they had just decided that underscore meant "protected" (the protected/private distinction is pretty useless). Would have made a lot of PHP4 code a bit more compatible.

Also, you shouldn't have many private methods in a class anyway. It's a sign that there is another class inside struggling to get out.

I would have said that PHP5 gives us exceptions, but that's about it. You can do OO just fine in PHP4. It's the reference syntax that is annoying, not the OO stuff.

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)

My first preference is for code that is readable without inline docs. Comments are a safety net measure - agood thing, but a sign that there is trouble elsewhere.

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)

Agreed.

yours, Marcus

[REMIYA]

This is exactly, what a bad coding practice is:

test_should_return_successful_response
test_should_display_thirty_fees_at_a_time
test_should_display_fees_ordered_by_name_in_ascending_order
test_should_display_fee_search_form
test_should_display_link_to_new_fee_screen
test_should_display_next_page_link_when_more_than_thirty_fees
test_should_not_display_previous_page_link

No comment!

[Luke Redpath]

This is exactly, what is bad coding practice:
No comment!

Perhaps you'd actually like to comment and say exactly is wrong with that - I'd genuinely love to hear why you think thats bad - I think its perfect - It describes EXACTLY what that specific part of my app that I'm testing should be doing. These are test case methods, so forget about keeping method names as short as possible - clarity is the name of the game here and nothing else. In fact, forget about them being methods - just think of them as runnable specifications - the fact that I'm using the Ruby Test:Unit framework (xUnit based) which uses classes and methods for structure is irrelevant.

As I mentioned before I'm following the BDD way of doing things, but bent to a Unit Testing framework (hence the need for test_ on each method and underscores). If I was writing this in rSpec (which I plan to use down the line when its stable), I'd do something like this:

context "Fee listing" do
  specify "should return successful response" do
    # assertions etc...
  end

  specify "should display thirty fees at a time" do
    # assertions etc...
  end

  # more specs for this context...
end 

Lovely. Read it. Read the context name followed by a spec - do you not see how it reads like an actual specification, except they are runnable and therefore proof that your app is working properly, as well as being documentation of your apps behaviour?. Do you not see what advantage this has over standard (and often pointless) comments?

http://rspec.rubyforge.org/ - read some of the examples and tutorials - then perhaps you might reconsider before hastily writing off an excellent programming practice.

[F4nat1c]

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

its the unluckiest cat you'll ever meet.

[Dean C]

I hate all these "What makes good code?" topics. What may be considered well-designed may not be considered good by another person. Everyone does things differently. To me good coding is about avoiding bad coding practices, not utilising good ones

[Luke Redpath]

I hate all these "What makes good code?" topics.

Why hate something that often leads to informative and interesting discussion and may help people learn how to be a better programmer? Being a good programmer is as much about using good practice as well as avoiding bad practice.

[REMIYA]

First. Making function and class names as long as possible does't make the code readable, just the contrary. It doesn't matter if it is a test project or not.

Second. There is no a single comment. Although it may be perfectly clear code for you now, I am sure that in just several months you will look at it and say "My God, what the hell this method had to do. What is this that should return thirty one, for what reason, etc."

[McGruff]

If I sat down to some code like LukeRedpath posted I'd heave a huge sigh of relief. First, there are tests. Second, they look like good tests. Each one is focussed on a specific point: "test_should_return_successful_response" and "test_should_display_thirty_fees_at_a_time" didn't get lumped in together for example. If the 30-fees test breaks, you've got the former to tell you that you are at least getting a response, just not the one you wanted. That helps to figure out exactly what's going wrong. You can rule out a total system breakdown.

You can also see at-a-glance exactly what the code is supposed to do just by reading the method names. That's a thousand time better than comments. Documentation always gets out of date but, as is often said, the tests never lie.

[REMIYA]

If a documentation gets out of date, that means the code itself has chages. So the test should change too.

And as I said long names don't make code readable. If a code is good no one would ever need these lenghty names.

Reading the long names takes time. Example:

 for($i=0;$i<count(blabla);$i ++){
      Looping etc.
   } 

 for($this_is_a_variable_and_should_increment_with_every_loop=0;$this_is_a_variable_and_should_increment_with_every_loop<count(blabla);$this_is_a_variable_and_should_increment_with_every_loop ++){
      Looping etc.
   } 

Oh, yes. I see you are eager to sit down at this code (the names are really self-explainable)

[Luke Redpath]

First. Making function and class names as long as possible does't make the code readable, just the contrary. It doesn't matter if it is a test project or not.

Second. There is no a single comment. Although it may be perfectly clear code for you now, I am sure that in just several months you will look at it and say "My God, what the hell this method had to do. What is this that should return thirty one, for what reason, etc."

As I suspected, you completely misunderstood what I posted. It isn't a test project - they ARE the tests, not the actual code I'm testing. The fact that the methods are named as they are means there is ABSOLUTELY NO NEED for comments, which was kind of my point. I know what the methods do - the method names couldn't be any more clearer. They don't return anything - they are TEST methods.

Theres really little point in continuing this unless you read what I posted again and look at the rSpec link I provided you. Somewhere along the line you have gotten you are wires crossed, because you really aren't understanding what I'm saying.

[McGruff]

If a documentation gets out of date, that means the code itself has chages. So the test should change too.

If you test first, you never touch the code until you have a failing test. If the requirements change, or haven't been fully expressed, the first thing you'd do would be to add to or edit the test case and only then attack the code until everything runs green. Unless it's something trivial, you'd want to run the full test suite regularly as well as the individual unit test: changing one class might break something else which uses it.

Actually, even "trivial" changes can have consequences you didn't realise. The more often you run the full test suite the better.

The big thing about this is that you've got a formal method of capturing requirements which binds these tightly to working code. Documentation only explains what you think the code should do; the tests tell you what it really does. If you edit the docs but don't update the code, or vice versa, there's nothing to tell you something's amiss.

[Luke Redpath]

If a documentation gets out of date, that means the code itself has chages. So the test should change too.

And as I said long names don't make code readable. If a code is good no one would ever need these lenghty names.

Reading the long names takes time. Example:

 for($i=0;$i<count(blabla);$i ++){
      Looping etc.
   } 

 for($this_is_a_variable_and_should_increment_with_every_loop=0;$this_is_a_variable_and_should_increment_with_every_loop<count(blabla);$this_is_a_variable_and_should_increment_with_every_loop ++){
      Looping etc.
   } 

Oh, yes. I see you are eager to sit down at this code (the names are really self-explainable)

Small, simple piece of advice. Learn about unit testing, learn about TDD (or BDD), then revisit this thread. It will be a revelation. Nobody is talking about creating actual production code with ridiculously long names. We are talking about descriptive unit tests. Get the whole idea of methods and classes out of your head. Focus on the tests and their purpose. The test code I posted are tests for a completely separate piece of code which I have NOT posted (for confidentiality reasons - its a piece of client software!) but what you can tell from the test case I posted is what that particular controller in my Rails app does, even though you don't have the actual controller code in front of you. Thats the point.

Yes, tests need to be maintained if code changes significantly but you will always know when tests are out of synch because you run your tests all the time. Comments aren't runnable, therefore they offer far less value and become out of synch my more easily.

[Gsheep]

Coders are not a seperate elitest group from the rest of the world, so the closer the code gets to the end user the easier it should be to comprehend. Classes are great to refactor problems to small units and then find and use the inevitable recurring components within a system.

If I'm wanting to start a car, I turn the key and push the pedal to go. If I want to play a CD, I put it in the CD player and press play. If I wan to initialise a piece of code ideally I'll be able to say startsystem() and that it. If every action in the system can have a clear purpose like startsystem() more people will use the system and more people will benefit. More or less all of the ideals in this thread are good practices and I think it is important that we keep striving towards this ideal of easy to read, maintainable code.

[pergesu]

This is exactly, what a bad coding practice is:
No comment!

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

You can look at Luke's code and find out exactly what the class is supposed to do.

Imagine you make some changes to your code, run your tests, and all of a sudden test_should_display_fees_thirty_one_to_sixty fail. Would you be able to guess what broke in your code? I would come to the conclusion that page two isn't showing fees 31-60. That gives me a pretty solid starting point, don't you think?

I think it's safe to say that if someone is using good testing practices, they're probably not writing horrible production code such as that ridiculous_index_for_a_loop_that_nobody_would_ever_use.

[REMIYA]

If a test fails, it fails. It doesn't matter if its called test_should_display_fees_thirty_one_to_sixty or just test_1.

We are talking about writing quality code. Not about what someone thinks is the best way to write his own code. If you write code for your own you may even call your variable and method and class names using a dialect form the east part of China. And may be don't use comments, it's just needless time spent. And may be even not write at all. Consumption is better

[kohoutek]

I hate all these "What makes good code?" topics. What may be considered well-designed may not be considered good by another person. Everyone does things differently. To me good coding is about avoiding bad coding practices, not utilising good ones

As someone who is just about to get into the programming side of things, I find this thread to be extremely useful and the input given of great importance.

To me good coding is about avoiding bad coding practices, not utilising good ones

That's a nice phrase but entirely useless for a programming novice. I'd rather read examples and hope there are more to come.

[McGruff]

If a test fails, it fails. It doesn't matter if its called test_should_display_fees_thirty_one_to_sixty or just test_1.

Actually it matters a great deal.

Suppose a mock TableGateway method getFees($from, $to) is being passed the wrong $from & $to args and the test is failing. A unit testing framework like SimpleTest will produce a precise failure message detailing the expected parameters, what was actually received, and the test method in which the failure occured. OK you could quickly find the failing assertion whatever the test method is called but the verbose version is much clearer. It instantly tells you what the aim of the test is. With a name like "test_1" it'll take time to figure out what is exactly is being tested. Explicit names are just as important in tests as they are elsewhere. Clarity is a huge part of what makes good code, I think. I'll often try to refactor tests to make them read better as docs.

[Pavel_Nedved]

• I like to see standard variables held in a centralized config.inc.php file somewhere off the public server. For me, I think it makes everything easier (especially changing things like a phone number say).
  
• Obviously plenty of white space, good code formatting, and plenty of commenting is necessary to make your code readable.
  
• Also consistency is a big thing. Don't use:
  
  if (...) echo "condition worked";
  
  and then on the next line use
  
  if (...) {
  echo "condition worked";
  }
  
  Be consistent! For me, that means always using the "{"'s.

Hope that helps,

Pavel

[burncycle]

What about the "abilities"..that is portability, maintainability and readability and meaningful comments of course there are more but were discussed above

[mattalexx]

Off Topic:

How many forum members does it take to change a light bulb?

-1 to change the light bulb and to post that the light bulb has been changed
-14 to share similar experiences of changing light bulbs and how the light bulb could have been changed differently
-7 to caution about the dangers of changing light bulbs
-27 to point out spelling/grammar errors in posts about changing light bulbs
-53 to flame the spell checkers
-41 to correct spelling/grammar flames
-6 to argue over whether it's "lightbulb" or "light bulb" ...
-another 6 to condemn those 6 as anal-retentive
-2 industry professionals to inform the group that the proper term is "lamp"
-15 know-it-alls who claim *they* are in the industry, and that "light bulb" is perfectly correct
-156 to email the participant's ISPs complaining that they are in violation of their "acceptable use policy"
-109 to post that this group is not about light bulbs and to please take this discussion to a lightbulb group
-203 to demand that cross posting to hardware forum, off-topic forum, and lightbulb group about changing light bulbs be stopped
-111 to defend the posting to this group saying that we all use light bulbs and therefore the posts *are* relevant to this group
-306 to debate which method of changing light bulbs is superior, where to buy the best light bulbs, what brand of light bulbs work best for this technique and what brands are faulty
-27 to post URL's where one can see examples of different light bulbs
-14 to post that the URL's were posted incorrectly and then post the corrected URL's
-3 to post about links they found from the URL's that are relevant to this group which makes light bulbs relevant to this group
-33 to link all posts to date, quote them in their entirety including all headers and signatures, and add "Me too"
-12 to post to the group that they will no longer post because they cannot handle the light bulb controversy
-19 to quote the "Me too's" to say "Me three"
-4 to suggest that posters request the light bulb FAQ
-44 to ask what is a "FAQ"
-4 to say "didn't we go through this already a short time ago?"
-143 to say "do a Google search on light bulbs before posting questions about light bulbs"
-1 forum lurker to respond to the original post 6 months from now and start it all over again....