5
\$\begingroup\$

I have been learning object oriented programming for quite a while. I understand many concepts about it but I quickly realized that I made a big mistake just reading theory and not doing any actual coding . There was no practise at all and when I started to write some code, I felt it was harder to think and come up with great ideas of what exactly my code should do before I blindly go to write some code.

I'm trying to improve my multilanguage class and this is how it works:

  1. It first checks if a language from the lang= GET parameter is set and if that value exists in possible languages array. I then set that language to the currentLang property and cookie to remember that language for a longer time. If I'm getting a language from $_GET, then I assume that the user manually switched to another language.

  2. Almost the same check is made in the second if statement, but this time checking if COOKIE['lang'] is set. This condition is needed to check if the language was set previously.

  3. If $_GET['lang'] or $_COOKIE['lang'] is not set then I assume that no language was chosen manually nor last time user visited page, inside that condition I check if current browser language value can be found in possible languages array if so then I set current language from browser language string. If language from browser HTTP_ACCEPT_LANGUAGE can't be found then I set current language to 'English' as a default option.

class Langs {

    // for detecting current language
    public $currentLang = '';

    // all possible languages
    private $allLangs = ['lt', 'en'];

    public function __construct() {

        // get first two letters from string
        $browserLang = substr($_SERVER['HTTP_ACCEPT_LANGUAGE'], 0, 2);

        // if language in url is set and if that language value exists in possible languages array and if language file exists
        if(isset($_GET['lang']) && in_array($_GET['lang'], $this->allLangs) && $this->_langFileExist($_GET['lang'])) {
            setcookie("lang", $_GET['lang'], time() + 3600 * 24 * 30);

            // store current language
            $this->currentLang = $_GET['lang'];

            // if language in cookie is set and if that language value exists in possible languages array and if language file exists
        } elseif(isset($_COOKIE['lang']) && in_array($_COOKIE['lang'], $this->allLangs) && $this->_langFileExist($_COOKIE['lang'])) {

            // store current language from COOKIE
            $this->currentLang = $_COOKIE['lang'];

        // if GET['lang'] or COOKIE['lang'] is not set
        } elseif(!isset($_GET['lang']) || !isset($_COOKIE['lang'])) {

            // tries to detect browser language, if browser language exists in possible languages array and if language file exists
            if(in_array($browserLang, $this->allLangs) && $this->_langFileExist($browserLang)) {

                // set current language from browser language
                $this->currentLang = $browserLang;
            } else {
                // if browser lang detection failed or if that language file doesnt exist
                // set it to english by default
                $this->currentLang = 'en';
            }
        }


    }

    private function _langFileExist($langFile) {
        if(file_exists('langs/'.strtolower($langFile.'.php')))
            return true;
    }
}

$langs = new Langs();
echo 'Current Language: '.$langs->currentLang;
require('langs/'.$langs->currentLang.'.php');

?>
<!DOCTYPE html>
<html>
<head>
    <title><?php echo $lang['title']; ?></title>
</head>
<body>
<h1><?php echo $lang['heading']; ?></h1>
<p>Description: <?php echo $lang['description']; ?></p>
<footer><?php echo $lang['copyright']; ?></footer>
</body>
</html>

I store language files in 'langs' directory and using a two-letter convention like 'ru', en', 'es' to identify languages. In every language file there are language values that are stored in a $lang array.

Finally, I instantiate the Langs class so I can access the currentLang property. Then, the currentLang value is placed inside require to load the language file.

It's a very simple class and I want to keep it as simple as possible. I just need advice from you of how I can improve that class in almost every aspect to make it faster, more efficient and readable. I'd like to hear from someone what I'm doing wrong, or what could be done better. To be more precise I want to know if I should separate logic that is inside __construct() method in smaller pieces? Maybe you can see ways of how can I simplify if-elseif or possibly reduce conditions?

| improve this question | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Good job on your first question. \$\endgroup\$ – Phrancis Mar 3 '16 at 3:44
  • \$\begingroup\$ Thanks :) I always try to do my best to explain everything that I need and with many details. \$\endgroup\$ – Erasus Mar 3 '16 at 11:51
2
\$\begingroup\$

The class is small and simple but I just made some changes for better reading.

  • I'm a fan of exit early so if an if statement is checked and there is nothing more to do, exit the function. the rest of the function is not necessary.
  • I also moved the duplicated check in_array($lang, $this->allLangs) && $this->_langFileExist($lang) in an own function, DRY.
  • Next thing is adding typehints for your IDE to detect your variable methods.
  • The last thing is to add a getter for $currentLang, I like this more than get a public attribute.

If you want, you can move each if statedments in the construct to an own function but in this case i would leave it like it is.

Hope this will help you.

<?php

/**
 * Class Langs
 */
class Langs 
{

    /** @var string $currentLang for detecting current language */
    private $currentLang = '';

    /** @var array $allLangs all possible languages */
    private $allLangs = ['lt', 'en'];

    /**
     * Langs constructor.
     */
    public function __construct() 
    {
        // get lang from url
        if(isset($_GET['lang']) && $this->_langIsAvailable($_GET['lang'])) {
            setcookie("lang", $_GET['lang'], time() + 3600 * 24 * 30);

            // store current language
            $this->currentLang = $_GET['lang'];

            return;
        }

        // get lang from cookie
        if(isset($_COOKIE['lang']) && $this->_langIsAvailable($_COOKIE['lang'])) {

            // store current language from COOKIE
            $this->currentLang = $_COOKIE['lang'];
            return;
        }

        // if no lang is set
        if(empty($this->currentLang)) {
            // get first two letters from string
            $browserLang = substr($_SERVER['HTTP_ACCEPT_LANGUAGE'], 0, 2);

            // tries to detect browser language, if browser language exists in possible languages array and if language file exists
            if($this->_langIsAvailable($browserLang)) {

                // set current language from browser language
                $this->currentLang = $browserLang;
                return;
            }
        }

        // set it to english by default
        $this->currentLang = 'en';
    }

    /**
     * @return string
     */
    public function getCurrentLang()
    {
        return $this->currentLang;
    }

    /**
     * @param string $lang
     * @return bool
     */
    private function _langIsAvailable($lang) 
    {
        return in_array($lang, $this->allLangs) && $this->_langFileExist($lang);
    }

    /**
     * @param string $lang
     * @return bool
     */
    private function _langFileExist($lang) 
    {
        return file_exists('langs/'.strtolower($lang.'.php'));

    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you! Now it really looks more readable and now it's really easy to determine the type of variables. Do you think it isn't overkill creating a few methods for such a simple class? I'm not sure if method calls are expensive in terms of performance, I need to learn more about it. Anyway two or more heads usually are better than one :) I can also see how return; statement can be useful, if I understand correctly this statement makes sure that no other checks will be executed because there's no point in doing it? \$\endgroup\$ – Erasus Mar 3 '16 at 12:05
  • 1
    \$\begingroup\$ @mc388 thank you for contributing your first answer. Please take a moment to explain what you have changed and why, as "code-only" answers are not as valuable from a code reviewing standpoint. See How do I write a good answer? \$\endgroup\$ – Phrancis Mar 3 '16 at 12:30
  • 1
    \$\begingroup\$ @Erasus In my opinion if a block of logic is repeated twice, you should try to abstract it. DRY is a core tenant of good programming, practice it as much as possible. Method calls are very cheap as far as performance goes. As for when to do the return statements, I do like mc388 's design process (+1 to you good sir), though know it is not the only option. \$\endgroup\$ – David J Eddy Mar 3 '16 at 17:38

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.