is this a good way of writing OOP

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
AshrakTheWhite
Forum Commoner
Posts: 69
Joined: Thu Feb 02, 2006 6:47 am

is this a good way of writing OOP

Post by AshrakTheWhite »

Just what the topic says:) opinnions please!
Main:

Code: Select all

<?php
require_once 'core.php';
require_once 'printer.php';

$print = new Printer;

echo $print->main_table;
?>
Core:

Code: Select all

<?php
session_start();
?>

Code: Select all

<?php
require_once 'core.php';
class Make_vars
{
    public $ostja_nimi;
    public $ostja_aadress;
    public $arve_nr;
    public $arve_kuupaev;
    public $maksetahtaeg;
    public $kirjeldus_nimi;
    public $kogus_nr;
    public $hind_nr;
    public $summa_nr;
    public $kaibemaks_nr;
    public $markused_nimi;
    
    public function __construct()
    {
        $this->ostja_nimi = $_POST['ostja_nimi'];
        $this->ostja_aadress = $_POST['ostja_aadress'];
        $this->arve_nr = $_POST['arve_nr'];
        $this->kirjeldus_nimi = $_POST['kirjeldus_nimi'];
        $this->kogus_nr = $_POST['kogus_nr'];
        $this->hind_nr = $_POST['hind_nr'];
        $this->markused_nimi = $_POST['markused_nimi'];
        $this->uhik_nimi = $_POST['uhik_nimi'];
    }

}

?>

Code: Select all

<?php
require_once 'make_vars';

class Math
{   
    private make_vars;

    public function __construct()
    {
        $this->make_vars = new Make_vars;
        $this->make_vars->kaibemaks_nr = 0.18;
    }
    

    public function arve_kuupaev()
    {   
        $this->arve_kuupaev = mktime(0, 0, 0, date("m"), date("d"),  date("Y"));
        return $this->arve_kuupaev;
    }

    public function maksetahtaeg()
    {
        $this->arve_kuupaev = $this->arve_kuupaev = mktime(0, 0, 0, date("m"), date("d") + 7,  date("Y"));
        return $this->maksetahtaeg;
    }
    
    public function getItemSum()
    {
        $out = $this->make_vars->hind_nr * $this->make_vars->kogus_nr;
        return $out;
    }

    public function getTotalSum()
    {
        $total = new array();
        $total = $this->getItemSum();
    }

}

?>
AshrakTheWhite
Forum Commoner
Posts: 69
Joined: Thu Feb 02, 2006 6:47 am

Post by AshrakTheWhite »

Code: Select all

<?php
require_once 'Core.php';
require_once 'Make_vars.php';
require_once 'math.php';

class Printer
{
    private $do_math;
    private $make_vars;
    public $main_table;
    public $input_table;

    public function __construct()
    {
        $this->make_vars = new Make_vars;
        $this->do_math = new Math;
        $this->main_table = $this->printer();
    }

    public function printer()
    {
        if(isset($this->make_vars->ostja_nimi, $this->make_vars->arve_nr, $this->make_vars->kirjeldus_nimi, $this->make_vars->kogus_nr, $this->make_vars->hind_nr, $this->make_vars->allahindlus_nr))
        {
            $out = '
                <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
                <html xmlns="http://www.w3.org/1999/xhtml">
                <head>
                <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
                <title>Arve</title>
                <link href="primary.css" rel="stylesheet" type="text/css" />
                </head>

                <body>
                <table width="800" border="1" cellpadding="0" cellspacing="0">
                   <tr>
                      <td width="308" height="133" valign="top"><p class="bold">Arve Nr. '. $this->make_vars->arve_nr .'</p>
                         <p>Arve kuupäev: '. $this->do_math->arve_kuupaev .'<br />
                            Maksetähtaeg: '. $this->do_math->maksetaehtaeg .'</p>
                      <p>&nbsp;</p>
                      <span class="bold">Ostja:</span></td>
                      <td width="292" rowspan="2" valign="top"><p class="allikiri"><br />
                         <br />
                         <span class="firma">NOTOS INVEST O&Uuml;
                         <br />
                         Reg. kood: 11033401<br />
                         S&auml;rgava allee 6 12013 Tallinn<br />
                telefon: 6420567<br />
                info@mugav.ee<br />
                             a/a: 10220036974019</span></p>
                      </td>
                   </tr>
                   
                   
                   <tr>
                      <td height="92" valign="top">'. $this->make_vars->ostja_nimi .'<br />
                      '. $this->make_vars->ostja_aadress .'
                      </td>
                   </tr>
                </table>
                <table width="800" border="1" cellpadding="0" cellspacing="0">
                   <tr>
                      <td width="400" height="20" valign="top">Nimetus</td>
                        <td width="100" valign="top">M&auml;rkused</td>
                        <td width="100" valign="top">Kogus</td>
                        <td width="100" valign="top">&Uuml;hik</td>
                        <td width="100" valign="top">Hind</td>
                        <td width="98" valign="top">Summa</td>
                   </tr>
                   '. .'
                   <tr>
                      <td width="400" height="20" valign="top">'. $this->make_vars->kirjeldus_nimi .'</td>
                        <td width="100" valign="top">'. $this->make_vars->markused_nimi .'</td>
                        <td width="100" valign="top">'. $this->make_vars->kogus_nr .'</td>
                        <td width="100" valign="top">'. $this->make_vars->uhik_nimi .'</td>
                        <td width="100" valign="top">'. $this->make_vars->hind_nr .'</td>
                        <td width="98" valign="top">'. $this->do_math->getItemSum .'</td>
                   </tr>
                </table>
                <table width="800" border="1" cellpadding="0" cellspacing="0">
                   <tr>
                      <td width="667" height="60" valign="top" class="bold2">Maksumus k&auml;ibemaksuta: <br />
                   K&auml;ibemaks 18%: <br />
                   Kokku:&nbsp;</td>
                   <td width="127" valign="top">var k&auml;ibemaks<br />
                      var 18%<br />
                      varkokku</td>
                   </tr>
                </table>
                <table width="800" border="1" cellpadding="0" cellspacing="0">
                   <tr>
                      <td width="800" height="206" valign="top"><p>Tasumisele kuulub: </p>
                         <p>&nbsp;</p>
                         <p>Pretensioonid t&ouml;&ouml; teostamise kohta esitada 24h jooksul peale t&ouml;&ouml; vastuv&otilde;tmist. </p>
                      <p>&nbsp;</p>         
                      <p class="allikiri">V&auml;ljastas: ................................... &nbsp;&nbsp;&nbsp;T&ouml;&ouml; ja arve k&auml;tte saadud: ..................................</p></td>
                   </tr>
                </table>
                </body>
                </html>
            ';
            return $out;
        } else
            {
                $out = '
                    <HTML>
                    <title>
                    Input the values Please!
                    </title>
                    <body>
                        <form action="Main.php" method="post">
                            Ostja Nimi:<input type=text name="ostja_nimi">
                            Ostja Aadress:<input type=text name="ostja_aadress">
                            arve number:<input type=text name="arve_nr">
                            kirjeldus:<input type=text name="kirjeldus_nimi">
                            kogus:<input type=text name="kogus_nr">
                            hind:<input type=text name="hind_nr">
                            Märkused:<input type=text name="markused_nimi">
                            Ühik:<input type=text name="uhik_nimi">
                            <input type="submit" name="submit" value="send">
                        </form>
                    </body>
                    </HTML>
                ';
        
                return $out;
            }    
    }
}
?>
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Post by alex.barylski »

2 comments:

1) Seeing how your name (alias/handle) implies racial cleansing...in particularly the race I happen to belong too :P

I'm not going to offer you much advice...

2) I don't really care actually, i'm just bugging, although I question your motives...

Your storing HTML inside an OOP class and your asking if it's proper design?

No...thats bad practice...and I'm not even going to get into why as I've argued my point a 100 times over...

Cheers :)
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post by shiznatix »

i would definatly seperate your HTML from your class if possible.

also I would not do this:

Code: Select all

public function __construct()
    {
        $this->ostja_nimi = $_POST['ostja_nimi'];
        $this->ostja_aadress = $_POST['ostja_aadress'];
        $this->arve_nr = $_POST['arve_nr'];
        $this->kirjeldus_nimi = $_POST['kirjeldus_nimi'];
        $this->kogus_nr = $_POST['kogus_nr'];
        $this->hind_nr = $_POST['hind_nr'];
        $this->markused_nimi = $_POST['markused_nimi'];
        $this->uhik_nimi = $_POST['uhik_nimi'];
    }
instead I would do something like this:

Code: Select all

public function __construct($nimi, $aadress, $nr, $client_name, $misOn_nr, $price_nr, $marked_name, $kuidas_name)
{
        $this->ostja_nimi = $nimi;
        $this->ostja_aadress = $aadress;
        $this->arve_nr = $nr;
        $this->kirjeldus_nimi = $client_name;
        $this->kogus_nr = $misOn_nr;
        $this->hind_nr = $price_nr;
        $this->markused_nimi = $marked_name;
        $this->uhik_nimi = $kuidas_name;
}
then when you call it be like:

Code: Select all

$classInstance = new Make_vars($_POST[].......);//fill in yer post stuff there
also, it would be easiest for other coders that might have to look at your stuff if you stuck to 1 language for your variable/class/function names instead of switching at random. English would be the prefered because not tooo many people outside of estonia know estonian ;)
AshrakTheWhite
Forum Commoner
Posts: 69
Joined: Thu Feb 02, 2006 6:47 am

Post by AshrakTheWhite »

Hockey wrote:2 comments:

1) Seeing how your name (alias/handle) implies racial cleansing...in particularly the race I happen to belong too :P

I'm not going to offer you much advice...
Ashrak - Gou'Uld assasin from the seriest stargate sg-1
TheWhite - Reference to Gandalf The White or Saruman the white from Lotr trilogy
2) I don't really care actually, i'm just bugging, although I question your motives...

Your storing HTML inside an OOP class and your asking if it's proper design?

No...thats bad practice...and I'm not even going to get into why as I've argued my point a 100 times over...

Cheers :)

Mind giving a link to wherey ouv argumented it? :)


EDIT: Also the argument names are in estonian because i have no freaking idea on how they translate to english :p
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

There are a couple of things. First is your Make_vars class is usually refered to as something like HTTP_Request because that's what it is. Since you are using PHP5, something like this might be better:

Code: Select all

class HTTP_Request {

    public function __get($key) {
        return isset($_POST[$key]) ? $_POST[$key] : null;
    }

}
And you Printer class is usually called something like View. Also it is usually better (for reuse, testing, etc.) to not have internal dependencies on instansiating classes, so something like this might be better:

Code: Select all

class View
{
    private $do_math;
    private $make_vars;
    public $main_table;
    public $input_table;

    public function __construct($request, $model)
    {
        $this->make_vars = $request;
        $this->do_math = $model;
        $this->main_table = $this->printer();
    }
Created like this:

Code: Select all

$view = new View(new Request, new Math);
Notice how things are falling out naturally into the MVC pattern.
(#10850)
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Re: is this a good way of writing OOP

Post by timvw »

AshrakTheWhite wrote:Just what the topic says:) opinnions please!
Please repeat your question in the body itself.. Anyway, before i can judge on your code, how could you define 'good way of writing OOP' ?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: is this a good way of writing OOP

Post by Christopher »

timvw wrote:Anyway, before i can judge on your code, how could you define 'good way of writing OOP' ?
I think the question, "Is this a good way of writing OOP?" implied that the OP wasn't sure what "good OOP" is. I'm not sure that I know, but luckily they just asked for opinions -- not facts.
(#10850)
AshrakTheWhite
Forum Commoner
Posts: 69
Joined: Thu Feb 02, 2006 6:47 am

Post by AshrakTheWhite »

thats my first try of OOP and my 2nd week of coding, so i need to develop standards so people would understand what the heck is going on ;)
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

Here's mine opinion: Did the code meet the customer requirements? If yes, then it's good (but there is room for improvement). If no, then it's not good.

Eg: What is the purpose of the following?? It doesn't return anything and it doesn't change anything... So i wonder ;)

Code: Select all

public function getTotalSum()
    {
        $total = new array();
        $total = $this->getItemSum();
    }
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

AshrakTheWhite wrote:thats my first try of OOP and my 2nd week of coding, so i need to develop standards so people would understand what the heck is going on ;)
I think it is a very good first try. Maybe getting a book or searching the net about some programming basics of dependencies, layering, modularization, etc. would be a good thing to do. I think if you read up on pattern, specifically on MVC in PHP, and just let it soak in knowing that you won't understand much at first would be good as well.
(#10850)
AshrakTheWhite
Forum Commoner
Posts: 69
Joined: Thu Feb 02, 2006 6:47 am

Post by AshrakTheWhite »

timvw wrote:Here's mine opinion: Did the code meet the customer requirements? If yes, then it's good (but there is room for improvement). If no, then it's not good.

Eg: What is the purpose of the following?? It doesn't return anything and it doesn't change anything... So i wonder ;)

Code: Select all

public function getTotalSum()
    {
        $total = new array();
        $total = $this->getItemSum();
    }

unfinished function :p

its supposed to add together all the sums from the previous function, yet to figure out how to do that :(
Post Reply