6
\$\begingroup\$

I would like advice on making my code more elegant and straightforward. The code works great, but it lacks these things.

<?php
if(isset($_POST['retrieve_posts'])) {
    $post_ret = $_POST['retrieve_posts'];
}
if(isset($_POST['retrieve_images'])) {
    $image_ret = $_POST['retrieve_images'];
} else $image_ret = 0;

require_once('classes.php');
require_once('functions.php');

if($post_ret == 1) {
    $obj = new kigo();
    $obj->url = "https://app.kigo.net/api/ra/v1/listProperties";
    $obj->user = "bla";
    $obj->pass = "bla";
    $obj->data = json_encode(null);
    $list = $obj->curlkigo();

    $directory = 'uploads';
    $list = $list['API_REPLY'];
    $c = count($list);
    $kigopropid = array();
    for($i=0;$i<$c;$i++) {
        $kigopropid[] = $list[$i]['PROP_ID'];
        $propname[] = $list[$i]['PROP_NAME'];
    }
 if(namecheck($kigopropid, $directory)!=null) {
$namecheck = namecheck($kigopropid, $directory);
$tau = 0;
foreach ($namecheck as $key => $prop_id) {
    $obj = new kigo();
    $obj->url = "https://app.kigo.net/api/ra/v1/readProperty";
    $obj->user = "bla";
    $obj->pass = "bla";
    $obj->data = json_encode(array("PROP_ID" => $prop_id));
    $obj->curlkigo();
    $data = $obj->curlkigo();

    //-----------Prop Name
    $title = $propname[$tau].'
    ';
    $tau++;

    //-----------Adress informations
    $strnr = unarr($data, 'PROP_STREETNO');
    $addr1 = unarr($data, 'PROP_ADDR1');
    $addr2 = unarr($data, 'PROP_ADDR2');
    $addr3 = unarr($data, 'PROP_ADDR3');
    $aptno = unarr($data, 'PROP_APTNO');
    $prop_postcode = unarr($data, 'PROP_POSTCODE');
    $prop_city = unarr($data, 'PROP_CITY');
    $prop_region = unarr($data, 'PROP_REGION');
    $prop_country = unarr($data, 'PROP_COUNTRY');
    $prop_lat = unarr($data, 'PROP_LATITUDE');
    $prop_long = unarr($data, 'PROP_LONGITUDE');
    $prop_axcode = unarr($data, 'PROP_AXSCODE');

    $adress = '
    <div class="adress">
        <h2>Adress</h2>
        <ul>
            <li>Primary Adress: '.$addr1.'</li>
            <li>Secondary adress: '. $addr2.'</li>
            <li>Tertiary adress: '.$addr3.'</li>
            <li>Street number: '. $strnr.'</li>
            <li>Apartment number: '. $aptno.'</li>
            <li>Postcode: '. $prop_postcode.'</li>
            <li>City: '. $prop_city.'</li>
            <li>Country: '. $prop_country.'</li>
            <li>Latitude: '. $prop_lat.'</li>
            <li>Longitude: '. $prop_long.'</li>
        </ul>
    </div>
    ';



    //-----------Property descriptions
    $name = unarr($data, 'PROP_NAME');
    $instant_book = unarr($data, 'PROP_INSTANT_BOOK');
    $metadescription = unarr($data, 'PROP_SHORTDESCRIPTION');
    $description = unarr($data, 'PROP_DESCRIPTION');
    $areadescription = unarr($data, 'PROP_AREADESCRIPTION');

    $properties = '
    <div class="content">
        <h2>'. $name.'</h2>
        <p>'.format($description).'</p>
    </div>
    ';

    //-----------Property details
    $prop_bedrooms = unarr($data, 'PROP_BEDROOMS');
    $prop_beds = unarr($data, 'PROP_BEDS');
    $prop_baths = unarr($data, 'PROP_BATHROOMS');
    $prop_toilets = unarr($data, 'PROP_TOILETS');
    $prop_size = unarr($data, 'PROP_SIZE').strtolower(unarr($data, 'PROP_SIZE_UNIT'))."s";
    $prop_floor = unarr($data, 'PROP_FLOOR');
    $prop_elevator = unarr($data, 'PROP_ELEVATOR');

    $details = '
    <div class="propdetails">
        <h2>Property details</h2>
        <ul>
            <li>Bedrooms: '.$prop_bedrooms.'</li>
            <li>Beds: '. $prop_beds.'</li>
            <li>Baths: '.$prop_baths.'</li>
            <li>Toilets: '. $prop_toilets.'</li>
            <li>Size: '. $prop_size.'</li>
            <li>Floor: '. $prop_floor.'</li>
            <li>Elevator: '. $prop_elevator.'</li>
        </ul>
    </div>
    ';

    //-----------Rates
    $nightly_rate_from = unarr($data, 'PROP_RATE_NIGHTLY_FROM');
    $nightly_rate_to = unarr($data, 'PROP_RATE_NIGHTLY_TO');
    $weekly_rate_from = unarr($data, 'PROP_RATE_WEEKLY_FROM');
    $weekly_rate_to = unarr($data, 'PROP_RATE_WEEKLY_TO');
    $monthly_rate_from = unarr($data, 'PROP_RATE_MONTHLY_FROM');
    $monthly_rate_to = unarr($data, 'PROP_RATE_MONTHLY_TO');
    $prop_rate_currency = unarr($data, 'PROP_RATE_CURRENCY');

    $rates = '
    <div class="rates">
        <h2>Rates</h2>
        <ul>
            <li>Nigtly rate from: '.$nightly_rate_from.'</li>
            <li>Nightly rate to: '.$nightly_rate_to.'</li>
            <li>Weekly rate from: '.$weekly_rate_from.'</li>
            <li>Weekly rate to: '.$weekly_rate_to.'</li>
            <li>Montly rate from: '.$monthly_rate_from.'</li>
            <li>Montly rate to: '.$monthly_rate_to.'</li>
            <li>Rate currency: '.$prop_rate_currency.'</li>
    </div>
    '; 

    //-----------Contact
    $prop_phone = unarr($data, 'PROP_PHONE');
    if($prop_phone==null) {$prop_phone = " - ";}
    $contact = '
    <div class="contact">
        <h2>Contact</h2>
        <p>'.$prop_phone.'</p>
    </div>
    ';

    if($image_ret==2) {
        //-----------Property Images
        $prop_array_img = unarr($data, 'PROP_PHOTOS');
        $img_ct = count($prop_array_img);
        $year = date('Y'); $month = date('m');
        for($i=0;$i<$img_ct;$i++) {
            $photo_id = $prop_array_img[$i]['PHOTO_ID'];
            $obj = new kigo();
            $obj->url = "https://app.kigo.net/api/ra/v1/readPropertyPhotoFile";
            $obj->user = 'bla';
            $obj->pass = 'bla';
            $obj->data = json_encode(array("PROP_ID" => $prop_id, "PHOTO_ID"=>$photo_id));
            $img = $obj->curlkigo();
            $img = str_replace(' ', '+', $img['API_REPLY']);
            $data = base64_decode($img);
            $file ="../../uploads/".$year."/".$month."/".uniqid() . '.jpg';
            $success = file_put_contents($file, $data);
        }
        echo "The Images were automatically added in media files!";
    } 

    $final = $title.$adress.$details.$rates.$properties.$contact;
    $create = fopen($directory.'/'.$prop_id.'.txt', 'w+');
    $put = file_put_contents($directory.'/'.$prop_id.'.txt', $final);
    }//end for
}//end if

$filenames = listfiles($directory);
if (file_exists($directory.'/archive.txt')) {
$filenames = array_values(array_diff($filenames, array('archive.txt')));
} 
$pathtozipfiles = array();
foreach ($filenames as $value) {
    $pathtozipfiles[] = $directory.'/'.$value;
}
$result = create_zip($pathtozipfiles,'articles.zip');
echo $dir;
}//end post
?>
\$\endgroup\$
3
  • \$\begingroup\$ Please do not remove your code after receiving answers. \$\endgroup\$ – Jamal Jan 20 '14 at 21:48
  • \$\begingroup\$ I hope that is not your real user/pass in 3 different places in the code, you should mask it and change the password as anyone can view the history for this post even if you change it now. \$\endgroup\$ – bumperbox Jan 21 '14 at 6:41
  • \$\begingroup\$ Nope, it was just in a test environement. \$\endgroup\$ – marianstefi20 Jan 22 '14 at 15:23
7
\$\begingroup\$

Yep not bad certainly seen worse

  • Try to keep your spacing consistant for example you do for() but foreach () one or the other
  • Doing this } else $image_ret = 0; is generally bad for readability / good code try to either use full if () { } else { } with the results of the if on separate lines or use ternary's where possible.
  • Try to segment off your process code from your display code so put your process code in functions/methods etc and have those accept the inputs and spit back vars then have your display side of things prepare the html and put your output into it. Ignore people telling you to use frameworks as they're all generally horrible anyway. Better off learning to write better code yourself and if required use something like twig for layout.
\$\endgroup\$
1
  • \$\begingroup\$ Frameworks are a good way of learning design patterns. I've never worked with one in a large scale project so I can't recommend their use but I do highly recommend looking over them for inspiration. \$\endgroup\$ – Sabrina Gelbart Dec 2 '13 at 3:42
6
\$\begingroup\$

It's not too bad.

  • First, make sure your indentation is always correct, eg. in your last if and your last foreach.
  • Second, never store passwords in your main source file! They should go in a file that is not version-controlled and only contains the login informations.
  • Last, you need to learn about separation of concerns: modern PHP code does not output HTML in the main logic functions: you should first prepare the data, then another function or class will output it. Learning to use a PHP framework is the best way to use those best practices.
\$\endgroup\$
5
\$\begingroup\$

See inline comments

<?php

// i have moved the requires to the top, as your class should fail as early as possible if these files don't exist
require_once('classes.php');
require_once('functions.php');

// these constants should be stored in a separate config file
const KIGO_USER = 'blabla';
const KIGO_PASS = 'blabla';
const DIR_UPLOADS = 'uploads';


//if(isset($_POST['retrieve_posts'])) {
//    $post_ret = $_POST['retrieve_posts'];
//}

// at this point $post_ret may not be set and will cause and error below when you test it
// a nicer way is to use the ternary operator to ensure it has a default value
$post_ret = isset($_POST['retrieve_posts']) ? $_POST['retrieve_posts'] : 0;


//if(isset($_POST['retrieve_images'])) {
//    $image_ret = $_POST['retrieve_images'];
//} else $image_ret = 0;

// same here with ternary operator
$image_ret = isset($_POST['retrieve_images']) ? $_POST['retrieve_images'] : 0;



if($post_ret == 1) {

    // the url, user, pass should all be constants or put into a helper class of sorts,
    // see mykigo helper class below
    //      $obj = new kigo();
    //    $obj->url = "https://app.kigo.net/api/ra/v1/listProperties";
    //    $obj->user = "bla";
    //    $obj->pass = "bla";
    //    $obj->data = json_encode(null);

    $mykigo = new mykigo(KIGO_USER, KIGO_PASS);

    // directory looks like config data and should be stored out of the main code unless it changes often
    // see const DIR_UPLOADS above
    // $directory = 'uploads';
    // $list = $list['API_REPLY'];
    $list = $mykigo->listProperties();

    $kigopropid = array();

    //    $c = count($list);
    //    for($i=0;$i<$c;$i++) {

    // i know people will say to do the count first so it is optimized better, but in reality count($list) will take such a tiny amount of time,
    // that i prefer it like this, as i find it more readable.
    for($i=0; $i<count($list); $i++) {
        $kigopropid[] = $list[$i]['PROP_ID'];
        $propname[] = $list[$i]['PROP_NAME'];
    }

    // namecheck is not a very obvious description of what the function does
    // for example, would something like this be more appropriate
    // if(is_directory_valid($kigopropid, $directory)!=null) {

    // here we are doing namecheck twice, what for?
    //    if(namecheck($kigopropid, $directory)!=null) {
    //        $namecheck = namecheck($kigopropid, $directory);

    // I am not sure what namecheck actually does, but this might be better
    $namecheck = namecheck($kigopropid, DIR_UPLOADS);
    if($namecheck != null) {

        $tau = 0;

        foreach ($namecheck as $key => $prop_id) {

            // $obj is such a generic name, it would be nicer to give it a name that means something
            // eg $kigo = new kigo();
            //            $obj = new kigo();
            //            $obj->url = "https://app.kigo.net/api/ra/v1/readProperty";
            //
            //            // i hope you haven't just posted your real username/password to the world
            //            $obj->user = "xxx";
            //            $obj->pass = "xxx";
            //
            //            $obj->data = json_encode(array("PROP_ID" => $prop_id));
            //
            //            // why do we call this twice, i am guessing it is a mistake?
            //            $obj->curlkigo();
            //            $data = $obj->curlkigo();

            // don't need to instantiate mykigo again, we have done that already, and can reuse it
            $data = $mykigo->readProperty($prop_id);

            //-----------Prop Name
            $title = $propname[$tau].'
        ';
            $tau++;

            // this code should be split into function, see below
            // i have done this one for example, I will leave the rest for you to do
            // by splitting into functions you will have a clear concise main function,
            // that should be easily readable

            $adress = render_address_to_html($data);

            //            //-----------Adress informations
            //            $strnr = unarr($data, 'PROP_STREETNO');
            //            $addr1 = unarr($data, 'PROP_ADDR1');
            //            $addr2 = unarr($data, 'PROP_ADDR2');
            //            $addr3 = unarr($data, 'PROP_ADDR3');
            //            $aptno = unarr($data, 'PROP_APTNO');
            //            $prop_postcode = unarr($data, 'PROP_POSTCODE');
            //            $prop_city = unarr($data, 'PROP_CITY');
            //            $prop_region = unarr($data, 'PROP_REGION');
            //            $prop_country = unarr($data, 'PROP_COUNTRY');
            //            $prop_lat = unarr($data, 'PROP_LATITUDE');
            //            $prop_long = unarr($data, 'PROP_LONGITUDE');
            //            $prop_axcode = unarr($data, 'PROP_AXSCODE');
            //
            //            $adress = '
            //    <div class="adress">
            //        <h2>Adress</h2>
            //        <ul>
            //            <li>Primary Adress: '.$addr1.'</li>
            //            <li>Secondary adress: '. $addr2.'</li>
            //            <li>Tertiary adress: '.$addr3.'</li>
            //            <li>Street number: '. $strnr.'</li>
            //            <li>Apartment number: '. $aptno.'</li>
            //            <li>Postcode: '. $prop_postcode.'</li>
            //            <li>City: '. $prop_city.'</li>
            //            <li>Country: '. $prop_country.'</li>
            //            <li>Latitude: '. $prop_lat.'</li>
            //            <li>Longitude: '. $prop_long.'</li>
            //        </ul>
            //    </div>
            //    ';



            //-----------Property descriptions
            $name = unarr($data, 'PROP_NAME');
            $instant_book = unarr($data, 'PROP_INSTANT_BOOK');
            $metadescription = unarr($data, 'PROP_SHORTDESCRIPTION');
            $description = unarr($data, 'PROP_DESCRIPTION');
            $areadescription = unarr($data, 'PROP_AREADESCRIPTION');

            $properties = '
        <div class="content">
            <h2>'. $name.'</h2>
            <p>'.format($description).'</p>
        </div>
        ';

            //-----------Property details
            $prop_bedrooms = unarr($data, 'PROP_BEDROOMS');
            $prop_beds = unarr($data, 'PROP_BEDS');
            $prop_baths = unarr($data, 'PROP_BATHROOMS');
            $prop_toilets = unarr($data, 'PROP_TOILETS');
            $prop_size = unarr($data, 'PROP_SIZE').strtolower(unarr($data, 'PROP_SIZE_UNIT'))."s";
            $prop_floor = unarr($data, 'PROP_FLOOR');
            $prop_elevator = unarr($data, 'PROP_ELEVATOR');

            $details = '
        <div class="propdetails">
            <h2>Property details</h2>
            <ul>
                <li>Bedrooms: '.$prop_bedrooms.'</li>
                <li>Beds: '. $prop_beds.'</li>
                <li>Baths: '.$prop_baths.'</li>
                <li>Toilets: '. $prop_toilets.'</li>
                <li>Size: '. $prop_size.'</li>
                <li>Floor: '. $prop_floor.'</li>
                <li>Elevator: '. $prop_elevator.'</li>
            </ul>
        </div>
        ';

            //-----------Rates
            $nightly_rate_from = unarr($data, 'PROP_RATE_NIGHTLY_FROM');
            $nightly_rate_to = unarr($data, 'PROP_RATE_NIGHTLY_TO');
            $weekly_rate_from = unarr($data, 'PROP_RATE_WEEKLY_FROM');
            $weekly_rate_to = unarr($data, 'PROP_RATE_WEEKLY_TO');
            $monthly_rate_from = unarr($data, 'PROP_RATE_MONTHLY_FROM');
            $monthly_rate_to = unarr($data, 'PROP_RATE_MONTHLY_TO');
            $prop_rate_currency = unarr($data, 'PROP_RATE_CURRENCY');

            $rates = '
        <div class="rates">
            <h2>Rates</h2>
            <ul>
                <li>Nigtly rate from: '.$nightly_rate_from.'</li>
                <li>Nightly rate to: '.$nightly_rate_to.'</li>
                <li>Weekly rate from: '.$weekly_rate_from.'</li>
                <li>Weekly rate to: '.$weekly_rate_to.'</li>
                <li>Montly rate from: '.$monthly_rate_from.'</li>
                <li>Montly rate to: '.$monthly_rate_to.'</li>
                <li>Rate currency: '.$prop_rate_currency.'</li>
        </div>
        ';

            //-----------Contact
            $prop_phone = unarr($data, 'PROP_PHONE');
            if($prop_phone==null) {$prop_phone = " - ";}
            $contact = '
        <div class="contact">
            <h2>Contact</h2>
            <p>'.$prop_phone.'</p>
        </div>
        ';

            if($image_ret==2) {
                //-----------Property Images
                $prop_array_img = unarr($data, 'PROP_PHOTOS');
                $img_ct = count($prop_array_img);
                $year = date('Y'); $month = date('m');
                for($i=0;$i<$img_ct;$i++) {
                    $photo_id = $prop_array_img[$i]['PHOTO_ID'];

                    // moved into mykigo helper class
                    $data = $mykigo->readPropertyPhotoFile($prop_id, $photo_id);

                    //                    $obj = new kigo();
                    //                    $obj->url = "https://app.kigo.net/api/ra/v1/readPropertyPhotoFile";
                    //                    $obj->user = "xxx";
                    //                    $obj->pass = "xxx";
                    //                    $obj->data = json_encode(array("PROP_ID" => $prop_id, "PHOTO_ID"=>$photo_id));
                    //                    $img = $obj->curlkigo();
                    //                    $img = str_replace(' ', '+', $img['API_REPLY']);
                    //                    $data = base64_decode($img);


                    $file ="../../uploads/".$year."/".$month."/".uniqid() . '.jpg';
                    $success = file_put_contents($file, $data);
                }
                echo "The Images were automatically added in media files!";
            }

            $final = $title.$adress.$details.$rates.$properties.$contact;
            $create = fopen(DIR_UPLOADS.'/'.$prop_id.'.txt', 'w+');
            $put = file_put_contents(DIR_UPLOADS.'/'.$prop_id.'.txt', $final);
        }//end for
    }//end if

    $filenames = listfiles(DIR_UPLOADS);
    if (file_exists(DIR_UPLOADS.'/archive.txt')) {
        $filenames = array_values(array_diff($filenames, array('archive.txt')));
    }
    $pathtozipfiles = array();
    foreach ($filenames as $value) {
        $pathtozipfiles[] = DIR_UPLOADS.'/'.$value;
    }
    $result = create_zip($pathtozipfiles,'articles.zip');
    echo $dir;


    function render_address_to_html($data) {

        //-----------Adress informations
        $strnr = unarr($data, 'PROP_STREETNO');
        $addr1 = unarr($data, 'PROP_ADDR1');
        $addr2 = unarr($data, 'PROP_ADDR2');
        $addr3 = unarr($data, 'PROP_ADDR3');
        $aptno = unarr($data, 'PROP_APTNO');
        $prop_postcode = unarr($data, 'PROP_POSTCODE');
        $prop_city = unarr($data, 'PROP_CITY');
        $prop_region = unarr($data, 'PROP_REGION');
        $prop_country = unarr($data, 'PROP_COUNTRY');
        $prop_lat = unarr($data, 'PROP_LATITUDE');
        $prop_long = unarr($data, 'PROP_LONGITUDE');
        $prop_axcode = unarr($data, 'PROP_AXSCODE');

        $adress = '
        <div class="adress">
            <h2>Adress</h2>
            <ul>
                <li>Primary Adress: '.$addr1.'</li>
                <li>Secondary adress: '. $addr2.'</li>
                <li>Tertiary adress: '.$addr3.'</li>
                <li>Street number: '. $strnr.'</li>
                <li>Apartment number: '. $aptno.'</li>
                <li>Postcode: '. $prop_postcode.'</li>
                <li>City: '. $prop_city.'</li>
                <li>Country: '. $prop_country.'</li>
                <li>Latitude: '. $prop_lat.'</li>
                <li>Longitude: '. $prop_long.'</li>
            </ul>
        </div>
        ';

        return $adress;
    }


}//end post

// i always remove the ending php closure, it has no use, and can actually mask whitespace getting output to page,
// which can effect the setting of cookies, etc

private class mykigo extends kigo {

    const URL_PROPERTY_PHOTO_FILE = 'https://app.kigo.net/api/ra/v1/readPropertyPhotoFile';
    const URL_LIST_PROPERTIES = 'https://app.kigo.net/api/ra/v1/listProperties';
    const URL_READ_PROPERTY = "https://app.kigo.net/api/ra/v1/readProperty";


    function __construct($user, $pass) {
        parent::__construct();

        $this->user = $user;
        $this->pass = $pass;
    }

    public function readProperty($prop_id) {
        $this->url = self::URL_READ_PROPERTY;
        $this->data = json_encode(array("PROP_ID" => $prop_id));
        return $this->curlkigo();
    }

    public function listProperties() {
        $this->url = self::URL_LIST_PROPERTIES;
        $this->data = json_encode(null);
        $ret = $this->curlkigo();
        return $ret['API_REPLY'];
    }

    public function readPropertyPhotoFile($prop_id, $photo_id) {
        $this->url = self::URL_PROPERTY_PHOTO_FILE;
        $this->data = json_encode(array("PROP_ID" => $prop_id, "PHOTO_ID"=>$photo_id));
        $ret = $this->curlkigo();
        $img = str_replace(' ', '+', $ret['API_REPLY']);

        return base64_decode($img);
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank's a lot...I know something about OOP but now I really saw it in action. It really makes the code great.Also, thanks for the great tips. \$\endgroup\$ – marianstefi20 Jan 25 '14 at 9:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.