SitePoint Sponsor

User Tag List

Page 1 of 8 12345 ... LastLast
Results 1 to 25 of 190
  1. #1
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

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

    Quote Originally Posted by CapitalWebHost
    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.
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  2. #2
    SitePoint Wizard stereofrog's Avatar
    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!

  3. #3
    SitePoint Addict
    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.

  4. #4
    SitePoint Wizard Ren's Avatar
    Join Date
    Aug 2003
    Location
    UK
    Posts
    1,060
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Use of interfaces. (Parameter typechecking and instanceof).
    Limited use of static method calls/singletons, which hinders testing/replacing.

  5. #5
    ********* Victim lastcraft's Avatar
    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() { ... }

    ...whilst this does not...
    PHP Code:
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  6. #6
    SitePoint Addict
    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:
    Quote Originally Posted by nacho
    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:
    PHP Code:
    // This does that 

  7. #7
    SitePoint Guru
    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!

  8. #8
    SitePoint Zealot Serberus's Avatar
    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.

  9. #9
    SitePoint Zealot agoossens's Avatar
    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.

  10. #10
    SitePoint Guru
    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.
    Logically speaking that is of course incorrect, given that there is a finite number of people on this planet
    I once had a problem.
    I thought: "Oh, I know: I'll just use XML!"
    Now I had two problems.

  11. #11
    simple tester McGruff's Avatar
    Join Date
    Sep 2003
    Location
    Glasgow
    Posts
    1,690
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by agoossens
    - 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...

  12. #12
    SitePoint Wizard stereofrog's Avatar
    Join Date
    Apr 2004
    Location
    germany
    Posts
    4,324
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Captain Proton
    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.

  13. #13
    SitePoint Wizard REMIYA's Avatar
    Join Date
    May 2005
    Posts
    1,351
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    It doesn't matter, if a cat is black or white, as long it catches mice .

  14. #14
    SitePoint Guru
    Join Date
    May 2003
    Location
    virginia
    Posts
    988
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by REMIYA
    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?

  15. #15
    SitePoint Wizard Young Twig's Avatar
    Join Date
    Dec 2003
    Location
    Albany, New York
    Posts
    1,355
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by mwmitchell
    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.

  16. #16
    SitePoint Wizard silver trophy kyberfabrikken's Avatar
    Join Date
    Jun 2004
    Location
    Copenhagen, Denmark
    Posts
    6,157
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by mwmitchell
    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 ...

  17. #17
    SitePoint Zealot kobra's Avatar
    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() { } 
    or

    PHP Code:
    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.


  18. #18
    SitePoint Guru
    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.

  19. #19
    SitePoint Wizard REMIYA's Avatar
    Join Date
    May 2005
    Posts
    1,351
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by kyberfabrikken
    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!

  20. #20
    One website at a time mmj's Avatar
    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

  21. #21
    Beer drinker Srirangan's Avatar
    Join Date
    Jan 2005
    Location
    Beerland!
    Posts
    776
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    More than the specifics of the code .. I'ld determine the quality of a system by its architecture ..
    Online Startups Insight for new entrepreneurs

  22. #22
    SitePoint Guru silver trophy Luke Redpath's Avatar
    Join Date
    Mar 2003
    Location
    London
    Posts
    794
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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;

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

    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 30assigns(: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 { |ff.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.

  23. #23
    SitePoint Addict
    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

  24. #24
    SitePoint Guru silver trophy Luke Redpath's Avatar
    Join Date
    Mar 2003
    Location
    London
    Posts
    794
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    This is the Ruby rSpec framework which gives a good overview of Behaviour Driven Development:

    http://rspec.rubyforge.org/

  25. #25
    SitePoint Enthusiast Buddha443556's Avatar
    Join Date
    Apr 2004
    Location
    FL, USA
    Posts
    87
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Srirangan
    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.
    Simple fool to the 3rd include.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •