Page 1 of 4

OO Usage Discussion

Posted: Thu Jun 12, 2008 2:36 am
by Benjamin
I'm just taking a break from writing some code and I wanted to discuss OO usage with you guys.

My question is this:

How many of you attempt to write 100% OO code, and for those who do, how do you feel about how this affects your productivity.

The reason I ask is because designing highly polymorphic classes can sometimes end up creating more development time, both short and long term due to an increase in the amount of code used to call the class, specifically in method arguments or configuration variables.

I think that there is a optimum balance between OO and procedural code that developers get a "feeling" for with experience.

Thoughts?

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 3:18 am
by dbevfat
I only write procedural code when prototyping quick and dirty standalone scripts. For everything else, I go OOP. I think that it improves productivity, and more importantly, maintainability. The complexity you describe is imo a code smell. If you're not using some method of testing (unit tests, tdd, bdd), I suggest you look into it; the side effect will be more clear and concise code and the code design will noticeably improve. I really don't think that good OOP will have that more code or method arguments than procedural code, in my experience it has less.

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 3:38 am
by Eran
I write entirely OO as well, it's all a matter of habit and experience with the techniques. It's important to develop a refactoring state of mind, and try not to write to far in advance - this usually ends with a lot of code not being used. Write code that works now and refactor later when you have good examples of different ways it can be used. I wrote about it a while ago - http://www.techfounder.net/2008/05/27/t ... factoring/

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 4:16 am
by Benjamin
dbevfat wrote:I only write procedural code when prototyping...
I've got some code that smells. There are times when I'm in a hurry or tired and I hack something together.

I just had a read about OOP at wikipedia which states that:
http://en.wikipedia.org/wiki/Object-oriented_programming wrote:Object-oriented programming may be seen as a collection of cooperating objects, as opposed to a traditional view in which a program may be seen as a group of tasks to compute ("subroutines"). In OOP, each object is capable of receiving messages, processing data, and sending messages to other objects.

Each object can be viewed as an independent little machine with a distinct role or responsibility. The actions or "operators" on the objects are closely associated with the object. For example, in OOP, the data structures tend to carry their own operators around with them (or at least "inherit" them from a similar object or "class"). The traditional approach tends to view and consider data and behavior separately.
So when I compare this to how I program I can see that I am using classes correctly. The major difference between me and most programmers however is the "style" in which I use them. I have been writing my own framework (like tons of other programmers have done, I know) in order to significantly increase development time. It's called the Accelerated Development Framework Environment or ADEF.

It contains a collection of useful classes, does not adhere to MVC, and provides unique ways to easily accomplish complicated tasks.

I've posted below a page from a site I built using the framework. This is the the contents of the entire register.php file. I have changed some of the field names. I posted this because it's the ugliest bit of code in the entire site. This page is the guts of a 3 step registration process where the URL does not change. ie the url is always http://site.com/register regardless of what the page is displaying. The first page asks the user for some basic information, the second page asks for some more and allows for an avatar upload (it captures the image even if other fields on the form fail validation and displays it), and the third page asks for some more information. The second and third pages can be skipped by clicking a link. I think that a lot of things are accomplished for only 207 lines of code.

Code: Select all

 
<?php
# cookies are required here
session_test($_KERNEL->paths->get('RESOLVED_REQUEST'), 'cookies');
 
# process registration skips
if (get('skip') != '')
{
    switch (get('skip'))
    {
        case '2':
            # display registration step 3
            $_KERNEL->compiler->change_destination('registration_step_3');
            return;
            break;
        case '3':
            # redirect to member's home page
            redirect('members/');
            break;
    }
}
 
# process any posted data
if (post('do_action') != '')
{
    switch (post('do_action'))
    {
        case 'new_registration':
            if ($_KERNEL->member_validation->new_registration())
            {
                # was the account created ok?
                if(false !== ($member_data = $_KERNEL->member_accounts->create_account()))
                {
                    # send welcome e-mail including activation code
                    $_KERNEL->email_notify->send_member_message($member_data, 'new_account_welcome');
                    
                    # log the user in
                    $_KERNEL->member_authentication->login($member_data['email_address']);
        
                    # display registration step 2
                    $_KERNEL->compiler->change_destination('registration_step_2');
                    return;
                } else {
                    # shouldn't end up here unless **** hits the fan someplace
                    $_KERNEL->status->error("An internal error has occurred.  Please try again.");
                }
            } else {
                # validation failure
                $_KERNEL->status->error($_KERNEL->form_errors->get_errors());
            }
            break;
        case 'save_step_two':
            # get the member data
            if (!$member_data = $_KERNEL->member_authentication->get_cur_user_data())
            {
                $_KERNEL->status->error("Your session data has expired.  Please login or restart the registration process.");
                redirect('register');
            }
 
            
            try {
                # process the file upload:
                $_KERNEL->upload->set_option('MAX_UPLOAD_SIZE', 6244640);
                $_KERNEL->upload->set_option('ALLOWED_EXTENSIONS', array('jpg', 'jpeg', 'gif', 'png'));
                $_KERNEL->upload->set_option('OVERWRITE_IF_EXIST', true);
                
                # create an image name
                $image_name = $member_data['member_id'] . '_' . SITE_ID . '_' . random_string(5);
                
                # if an image was uploaded process it
                if ($_FILES['avatar']['error'] != 4)
                {
                    # if an existing image exists, delete it if this upload succeeds
                    if (!empty($member_data['avatar']))
                    {
                        $old_avatar = $member_data['avatar'];
                    }
 
                    # save the uploaded image
                    if (!$temp_image = $_KERNEL->upload->save_file('avatar', AVATAR_SYS_PATH, "$image_name.tmp"))
                    {
                        throw new Exception($_KERNEL->upload->get_error());
                    }
    
                    # resize the uploaded image
                    if (false === ($resized = $_KERNEL->resize_image->process_resize(AVATAR_MAX_HEIGHT, AVATAR_MAX_WIDTH, AVATAR_SYS_PATH.$temp_image, AVATAR_SYS_PATH."$image_name.jpg")))
                    {
                        throw new Exception("There was a problem resizing your image.  Please try again or try a different image.");
                    }
                    
                    list($new_width, $new_height) = $resized;
                    unset($resized);
                    
                    # delete the tmp image
                    if (file_exists(AVATAR_SYS_PATH.$temp_image))
                    {
                        unlink(AVATAR_SYS_PATH.$temp_image);
                    }
                    
                    # delete the old image
                    if (file_exists(AVATAR_SYS_PATH.$old_avatar) && $old_avatar != "$image_name.jpg")
                    {
                        unlink(AVATAR_SYS_PATH.$old_avatar);
                    }
                    
                    # set the permissions
                    chmod(AVATAR_SYS_PATH."$image_name.jpg", '0644');
                    
                    # everything is ok with the image, we can save it now
                    $data = array('avatar'    => "$image_name.jpg",
                                  'av_width'  => $new_width,
                                  'av_height' => $new_height);
                    $_KERNEL->member_data->update($member_data['email_address'], $data);
                }
                
            } catch (Exception $e) {
                # clean up
                if (file_exists(AVATAR_SYS_PATH.$temp_image))
                {
                    unlink(AVATAR_SYS_PATH.$temp_image);
                }
                
                # set the error messages
                $_KERNEL->status->error($e->getMessage());
            }
            
            try {
                # validate the rest of the form
                if (!$_KERNEL->member_validation->registration_step_2())
                {
                    $_KERNEL->status->error($_KERNEL->form_errors->get_errors());
                    throw new Exception();
                }
                
                # update the members data in db
                $data = array('field_one'    => post('field_one'),
                              'field_two'    => post('field_two'),
                              'field_three'  => post('field_three'),
                              'field_four'   => post('field_four'));
                
                # this is sent through the member_data class which will also update memcache
                $_KERNEL->member_data->update($member_data['email_address'], $data);
                
                # display registration step 3
                $_KERNEL->compiler->change_destination('registration_step_3');
                return;            
            } catch (Exception $e) {
                
                # set the error messages
                $_KERNEL->status->error($e->getMessage());
 
                # redisplay registration step 2
                $_KERNEL->compiler->change_destination('registration_step_2');
                return;            
            }
            break;
        case 'save_step_three':
            # get the member data
            if (!$member_data = $_KERNEL->member_authentication->get_cur_user_data())
            {
                $_KERNEL->status->error("Your session data has expired.  Please login or restart the registration process.");
                redirect('register');
            }
            
            try {
                # validate the rest of the form
                if (!$_KERNEL->member_validation->registration_step_3())
                {
                    throw new Exception();
                }
                
                # remove any and all html
                $_POST = array_map('no_html', $_POST);
                
                # update the member_profile table
                $data = array('field_one'   => compress(post('field_one')),
                              'field_two'   => compress(post('field_two')),
                              'field_three' => compress(post('field_three')),
                              'field_four'  => compress(post('field_four')),
                              'field_five'  => post('field_five'),
                              'field_six'   => post('field_six'),
                              'field_sev'   => compress(post('field_sev')));
                
                $_KERNEL->db->update($data, 'member_profile', array('member_id' => "= {$member_data['member_id']}"));
                
                # redirect to member's home page
                redirect('members/');
            } catch (Exception $e) {
                # set the error messages
                $_KERNEL->status->error($_KERNEL->form_errors->get_errors());
 
                # redisplay registration step 2
                $_KERNEL->compiler->change_destination('registration_step_3');
                return;            
            }
            break;
    }
} else {
    $_POST['country_id'] = '223';
}
 
# the authentication mechanism requires that the user is logged
# out before an account can be created.
if ($member_data = $_KERNEL->member_authentication->get_cur_user_data())
{
    $_KERNEL->compiler->change_destination('register_must_logoff');
    return;
}
?>
<h1>Register New Account</h1>
 
<form name="register" method="post" action="register">
  <table class="form" cellspacing="0">
    <tr>
      <td class="label"><label for="first_name">First name:</label></td>
      <td><?php echo input('first_name'); ?></td>
    </tr>
    <tr>
      <td class="label"><label for="last_name">Last name:</label></td>
      <td><?php echo input('last_name'); ?></td>
    </tr>
    <tr>
      <td class="label"><label for="email_address">Email address:</label></td>
      <td><?php echo input('email_address'); ?></td>
    </tr>
    <tr>
      <td class="label"><label for="password">Password:</label></td>
      <td><?php echo password('password'); ?></td>
    </tr>
    <tr>
      <td class="label"><label for="confirm_password">Retype Password:</label></td>
      <td><?php echo password('confirm_password'); ?></td>
    </tr>
    <tr>
      <td class="label">Date of Birth:</td>
      <td><?php echo select('month', get_month_array()); ?> / <?php echo select('day', get_date_array()); ?> / <?php echo select('year', get_year_array()); ?>&nbsp;&nbsp;</td>
    </tr>
    <tr>
      <td class="label"><label for="country_id">Country:</label></td>
      <td><?php echo select('country_id', get_country_array()); ?></td>
    </tr>
    <tr>
      <td class="label"><label for="postal_code">Postal code:</label></td>
      <td><?php echo input('postal_code'); ?></td>
    </tr>
    <tr>
      <td class="label"><label for="gender">Gender:</label></td>
      <td><?php echo radio('gender', 'male', 'Male'), radio('gender', 'female', 'Female'); ?></td>
    </tr>
    <tr>
      <td class="label">&nbsp;</td>
      <td><?php echo checkbox('tos_agree', 'I have read and agree to the <a href="tos" title="View our terms of service">terms of service</a>'); ?></td>
    </tr>
    <tr>
      <td colspan="2" class="submit">
        <input type="hidden" name="do_action" value="new_registration" />
        <button type="submit">Sign Up</button>
      </td>
    </tr>
  </table>
</form>
 
Now as you can see, I'm using classes (reusable) to do a lot of the grunt work, but I haven't gone purely OO on this page because I think a majority of the sequences on this page are a one shot deal. Generally I don't put something into a class if I don't see any benefit from making it reusable. There have of course been times when I was wrong, in which case I refactored the code.

Just so the code is easier to understand.
$_KERNEL->classname calls a class, if the class isn't instantiated yet it is on the fly, passing arguments to the construct is not possible.
$_KERNEL->compiler->change_destination('page_name') changes the http request. So if you request index and change it to contact, the contact page is processed and displayed without redirecting, and index is still displayed in the url
$_KERNEL->status-> sets a success or error message, this message will be displayed even if you change_destination or do a hard redirect
The form_errors class tracks which fields contain errors and these fields are automatically assigned an error class via there respective function calls, ie input for an input box, select for a select menu.

pytrin wrote:I write entirely OO as well...
I read your article. I would say that I write code to be as simple as possible, sometimes to the extreme, but I definitely do not write code to refactor it. The last thing I want to do is rewrite code I have already written. Not because I don't want to touch code that "works" but rather I don't want to waste time. I'm not saying I don't or won't, I just try to avoid it. I'd push doing it right the first time over refactoring any day.

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 5:06 am
by superdezign
astions wrote:I read your article. I would say that I write code to be as simple as possible, sometimes to the extreme, but I definitely do not write code to refactor it. The last thing I want to do is rewrite code I have already written. Not because I don't want to touch code that "works" but rather I don't want to waste time. I'm not saying I don't or won't, I just try to avoid it. I'd push doing it right the first time over refactoring any day.
I completely agree. I program in OO completely, and I program in the abstract a lot, so when I don't plan ahead, programming those things takes a long time. So, now I always design my front-end, then design my back-end to fit it. But, failure to design ahead of time does extend the production time unnecessarily.

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 5:23 am
by Eran
In my opinion trying to "get it right the first time" is a nice concept but fails in practice unless you are very familiar with the problem domain. Trying to over design new solutions usually leads to much bloated code that never gets used (even if it seems it will be useful someday). From my experience I found that I save much more in development time when I don't try to anticipate every possible use-case and just focus on the use-cases that are going to be used for certain. I don't write to refactor, but I definitely don't shy away from it and it helps me avoid over-design.

Also from looking at your code, it seems pretty procedural in nature. The logic inside those switching cases could probably be encapsulated in different methods, and some of it could probably be broken down into even smaller components. Personally if I have a procedural code longer than 15-20 lines, I know its time to refactor into smaller methods. http://c2.com/cgi/wiki?LongMethodSmell

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 5:33 am
by Benjamin
pytrin wrote:In my opinion trying to "get it right the first time" is a nice concept but fails in practice unless you are very familiar with the problem domain.
You are right about that.
pytrin wrote:Trying to over design new solutions usually leads to much bloated code that never gets used
I'm not sure why you keep leaning towards overdesign. I think what I was getting at was designing something, then having to add features later.
pytrin wrote:Also from looking at your code, it seems pretty procedural in nature.
I think one could argue that the entire page is procedural, but there are no other pages like it. Of course I could write a class for it, but why would I want to? Almost every line in the file is using a specific class. This is why I brought up the topic.

If you checkout lines 28 through 40, there's only 5 actual lines of code there. These five lines:

1. Validate all the form fields
2. Create the member account
3. Send a welcome email
4. Log the user in
5. Render the 2nd step of the registration process

Those 5 classes or actions if you will are completely reusable. Calling them in that sequence isn't, or at least I don't predict it will be, so I did not classify it.

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 5:50 am
by Christopher
I am really not sure what you are asking Astions. Looking at your code, it looks similar to the pre-OOP version of every single-programmer PHP system ever created. It is really not OOP at all. Sure you are using the class construct, but essentially just namespacing your procedural function library with $_KERNEL. I could comment on lots of things -- like mixing application and template code in the same script -- but you seem pretty sold on your current ideas.

I will bet that this time next year you will be using code that is more OOP and will have moved toward an architecture more like the major PHP frameworks use.

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 5:51 am
by Mordred
First impressions (from reading it once) is that lower-level actions are well encapsulated in more or less single method calls, and the complexity of the code comes only from the inherent complexity of processing the 3-step registration process. I would imagine that the avatar upload can indeed be packed into a reusable class as well.

Overall, it looks okay to me. What are the "smells" you personally see in this snippet?

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 6:05 am
by Benjamin
arborint wrote:I will bet that this time next year you will be using code that is more OOP and will have moved toward an architecture more like the major PHP frameworks use.
Yes, the php and html is mixed, intentionally. I originally designed the system with them separated but it only slowed me down. It simply isn't possible to completely separate logic & display code. From what I have seen a majority of the frameworks simply mask it with a pseudo language.

I'm on a constant quest to improve my code, how I write it, design it and organize it. I have the mindset that "there is a better way" and I am bound and determined to find it. I'll be disappointed if your prediction is correct, but I don't think it will be. You may see horrible style, but it's very robust, very efficient, easy to modify and if I had to guess I would say that development time is reduced by over 50%. Oh yeah, the php files share the same memory scope as the real template too :twisted:

Keep in mind that you're only seeing a pinhole, there's much more to the framework than meets the eye from this single file. You can call any of those methods, even the change_destination and redirect from anyplace. Even after the html. I'm sure you noticed that there are no includes too. Anywhere, with the exception of the bootstrap.
Mordred wrote:Overall, it looks okay to me. What are the "smells" you personally see in this snippet?
1. There is login check code copied and pasted 2 or 3 times. Due to a change elsewhere there is now a bool variable which flags the login status, so I can replace that.
2. The new_registraion case has a nested if condition which could be combined with the first one.
3. I don't like moving the uploaded file to the avatar directory before it is resized. As an afterthought I might be able to resize it in the tmp directory.
4. I don't think multi-action pages ever look pretty.

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 6:22 am
by Eran
I'm not sure why you keep leaning towards overdesign. I think what I was getting at was designing something, then having to add features later.
Over design is something I see a lot with inexperienced OO programmers. They try to abstract everything using a very deep class and method hierarchy. It might not apply to you, but I felt the need to emphasis it since looking at your code you don't have much experience with OOP.

Multi page registration code can look much better once you abstract the basic building blocks of the multi page form. They don't necessarily belong in the same class (most likely they don't), and this is where an MVC structure helps a lot (using view helpers, putting form-step logic and redirections in the controllers and domain logic in the models).

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 6:24 am
by Benjamin
pytrin wrote:since looking at your code you don't have much experience with OOP.
Or I just think the way it's currently used by most programmers sucks. If there's a better way I'm all for it so if you think you can prove me wrong do it. I'm the first to admit when I'm wrong, although sometimes it does take some convincing.

You didn't qualify that statement. Are you saying I'm not experienced because I'm not passing objects to other objects? I've never had to do that and I've built some pretty complex systems. If I tell a class to do something I just want to know if it did it or not. I don't want to have to test to see if it returned an error object and then pass that error object to another class to figure out what the error is. If it returns false and I need to know I'll ask it, not something else.

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 6:36 am
by Eran
I don't know about most programmers, but I've always tried to learn from people who are considered the best at OO modeling and concepts (Martin Fowler, Kent Beck etc.). Studying design patterns and looking at implementations in high quality OO open-source packages (such as the major frameworks).

I don't think that all of that accumulated experience and expertise 'sucks'. Why reinvent the wheel when you can build on the work of others?

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 6:43 am
by superdezign
pytrin wrote:I don't think that all of that accumulated experience and expertise 'sucks'. Why reinvent the wheel when you can build on the work of others?
Who's the say that the current wheel rolls the best if no one tries a different design? ;)

Re: OO Usage Discussion

Posted: Thu Jun 12, 2008 6:47 am
by Benjamin
I've read:

php|architect's Guide to PHP Design Patterns by Jason Sweat
PHP 5 Objects, Patterns, and Practice by Matt Zandstra
Advanced PHP Programming by George Schlossnagle
MySQL Cookbook by Paul DuBois
Web Standards Solutions by Dan Cederholm
Bulletproof Web Design by Dan Cederholm
Web Systems Design and Online Consumer Behavior by Yaun Gao

among others. I learn and absorb what I am exposed to but I'm not going to jump on the bandwagon if I think I can do better, and so far the results have been acceptable to me, other than being looked down upon here on devnet because I'm different.

At my last programming position I was outperforming other programmers by a 3:1 margin.