1
\$\begingroup\$

I'm receiving an language string out of an PHP file which returns an array like this:

DIR_LANG\en\home.php:

return array(

    'welcome_string'    => 'welcome @ home! '

);

And my Lang class:

<?php

class Lang
{

    private static $langArray = array();
    private static $lang = DEFAULT_LANG;

    public static function get($langString)
    {
        if (isset(self::$langArray[self::$lang]) && isset(self::$langArray[self::$lang][$langString])) {
            return self::$langArray[self::$lang][$langString];
        } else {
            $langUsed = self::$lang;
            self::receiveLang($langString);

            if (!isset(self::$langArray[$langUsed][$langString])) {
                if ($langUsed != DEFAULT_LANG) {
                    $langUsed = DEFAULT_LANG;
                    self::receiveLanguage($langString, DEFAULT_LANG);
                } else {
                    $langUsed = FALLBACK_LANG;
                    self::receiveLanguage($langString, FALLBACK_LANG);
                }
            }
            return (isset(self::$langArray[$langUsed][$langString]) ? self::$langArray[$langUsed][$langString] : $langString);
        }
    }

    private static function receiveLang($langString)
    {
        $langStringExp = explode('.', $langString);
        $usedLang = self::$lang;
        $langFile = DIR_LANG . $usedLang . '/' . $langStringExp[0] . '.php';

        if (!file_exists($langFile)) {
            $usedLang = ($usedLang != DEFAULT_LANG) ? DEFAULT_LANG : FALLBACK_LANG;
            $langFile = DIR_LANG . $usedLang . '/' . $langStringExp[0] . '.php';

            if (!file_exists($langFile)) {
                return;
            }
        }
        $langFileContents = require_once $langFile;

        foreach ($langFileContents as $key => $value) {
            self::$langArray[$usedLang][$langStringExp[0] . '.' . $key] = $value;
        }
    }

    private static function receiveLanguage($langString, $language)
    {
        $langStringExp = explode('.', $langString);
        $langFile = DIR_LANG . $language . '/' . $langStringExp[0] . '.php';

        if (!file_exists($langFile)) {
            return;
        }

        $langFileContents = require_once $langFile;

        foreach ($langFileContents as $key => $value) {
            self::$langArray[$language][$langStringExp[0] . '.' . $key] = $value;
        }
    }

    public static function setLang($newLang)
    {
        self::$lang = $newLang;
    }
}

And somewhere in my view, I receive the language like this:

echo Lang::get('home.welcome_string');

The constants are defined in an config.php. Here's an stripdown of it:

define('DIR_LANG', 'app/language/');
define('DEFAULT_LANG', 'nl');
define('FALLBACK_LANG', 'en');

Although this do work, it feels a bit bloated. What could I do to improve this, or is this the best way (I can't believe it)

\$\endgroup\$
2
\$\begingroup\$

Before presenting my solution, I would like to quickly review your code. The first thing that popped into my mind was the extensive use of the keyword static, which I will address later together with your use of constants.

When looking over your code I have a hard time determining which variables and methods are different. All your variables start with the same characters. Your methods receiveLang() and receiveLanguage() easily creates confusion. What is it one does which the other don't? You also has a "silly assignment" where you write: $langUsed = self::$lang;. Since self::$lang is never changed you could use the static property and save a local variable. With that said I think your overall code looks good and your indentation is good. You could give more space and blank lines to increase readability though.

Pro Tip:

When using isset() you can reduce some of your conditionals to half the length. The difference between array_key_exists() and isset() is that isset() keeps going through keys even with multi-dimensional arrays. This means you could as an example reduce your first if condition in get() from:

if (isset(self::$langArray[self::$lang]) && isset(self::$langArray[self::$lang][$langString])) {...

to:

if(isset(self::$langArray[self::$lang][$langString])) {...

Now I would like to ask a question: why is your class completely static?. I know it's easier to interact with, but it also make testing your class harder.

I like your idea of using a file for each supported languages as it keeps concerns clear. But your use of constants that are defined outside your class, but is required lights some red warning signals to me. Consider me (another developer) or yourself in six months using this class. I could deduce that I need to provide a default path to languages files, but the default languages as well as the fallback language itself isn't so evident.

Considering your language class would only be used in a single view (right?), I would make this an instance class and provide the required constant values as parameters during construction to a __construct() method. Now if I would use your class I have to provide the required information. You would also have the benefit of (in some weird situation) having more than one language in your view as each instance would specify its preferred language.

Making the class an instance of course makes it more complicated to pass around the same instance, but it should be possible with a simple dependency manager.

I have written an alternative implementation of your class. It also implements a fallback language. The difference is that the solution I have written loads the fallback language first and then overwrites any language keys from the selected language. This makes the method for retrieving a language string much easier.

class Language {

    /**
     * @var array $language
     */
    private $language = [];

    /**
     * Create a Language instance.
     *
     * @param string $path     An absolute directory path to language files
     * @param string $used     A string declaring the language to use.
     * @param string $fallback A string declaring the default language.
     */
    public function __construct($path, $use, $default)
    {
        $this->language = $this->prepare($path, $use, $default);
    }

    /**
     * Determine if a language string exists.
     *
     * @param string $key
     *
     * @return bool
     */
    public function exists($key)
    {
        return array_key_exists($key, $this->language);
    }

    /**
     * Fetches the value of a language string associated with
     * the specified key.
     *
     * @param string $key
     *
     * @return string
     */
    public function fetch($key)
    {
        if($this->exists($key)) {
            return $this->language[$key];
        }

        /*
         * Throw an exception (preferred) or trigger an error/warning here; its your choice.
         *
         * I return an empty string to keep the return type consistent. You 
         * can return NULL, but you would have to check the returned value
         * each time.
         */
        return '';
    }

    /**
     * Prepares the class by loading specified languages and 
     * combining them into a single associative array.
     *
     * @param string $path
     * @param string $specified
     * @param string $fallback
     *
     * @return array
     */
    private function prepare($path, $use, $fallback)
    {
        /*
         * Load entire fallback language contents.
         */
        $language  = $this->load($path, $fallback);
        $overwrite = $this->load($path, $use);

        /*
         * I merge the language file recursively. The fallback language is
         * overwritten by the specified language.
         */
        return array_merge_recursive($language, $overwrite);
    }

    /**
     * Loads a language file and validates the returned contents
     * to an array.
     *
     * @param string $path
     * @param string $language
     *
     * @return array
     */
    private function load($path, $language)
    {
        /*
         * Build language filename.
         * An example format is: root/resources/languages/en.php
         */
        $filename = rtrim($path, '/') . '/' . strtolower($language) . '.php';

        if(!is_file($filename) || !is_readable($filename)) {
            throw new \RuntimeException("Cannot open language file: {$filename}");
        }

        $contents = require $filename;

        if(!is_array($contents)) {
            throw new \LogicException('Invalid language file structure. Expected an array.');
        }

        return $contents;
    }

}

This is a very crude implementation. You could easily build upon this, but the basics are there. You could even increase testability even more by extrating the load() and prepare() methods into a separate class. This class should only be responsible for loading your language files. You would then pass the loaded language as a parameter into the constructor

public function __construct(array $language) {...

Now consider testing this. You can now pass an array without loading any files and still test your class easily.

$array = [
    'welcome' => 'Welcome to this awesome site!'
];

$language = new Language($array);

I hope this can help you, happy coding!

\$\endgroup\$
  • \$\begingroup\$ First things first, Thank you very much for you answer. And thank you for your pro tip. Thought I had to do it that way because of (maybe) invalid indexes, it's never to late to learn. But your use of constants that are defined outside your class, but is required lights some red warning signals to me I did that because it looked 'logical' to me to declare all "config" variables in 1 files. But on the other hand, they belong with the language, so why don't we put it there... \$\endgroup\$ – Mathlight Aug 4 '15 at 17:33
  • \$\begingroup\$ And thank you, this do help me very, very much. It is and feels way better than my own implementation. \$\endgroup\$ – Mathlight Aug 4 '15 at 17:34
  • \$\begingroup\$ @Mathlight - You are welcome. The reason I talked about constants is that there are many (and I mean A LOT) of constants already in PHP. So to avoid "polluting" the global namespace even more and to better reflect your class's required attributes, I thought it logical to include them. \$\endgroup\$ – AnotherGuy Aug 4 '15 at 17:41
  • \$\begingroup\$ Yeah, it just never came up to me that i could just include it in the class or something like that. Thank you very much \$\endgroup\$ – Mathlight Aug 4 '15 at 17:56
  • 1
    \$\begingroup\$ You beat me to it. But +10! You did a really amazing job and it is quite a clean code. I wouldn't do any better. The only change I suggest is that you make a static private variable to hold all the languages' translations. This is to allow to use multiple instances, create new ones or delete, without the need to re-load the files and translations, saving you some CPU cycles. RAM usage is way less expencive than CPU power, so, you can relax a little on it. \$\endgroup\$ – Ismael Miguel Aug 4 '15 at 19:31

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.