2
\$\begingroup\$

At the moment I am trying to create a Japanese rehearsal system, and it's working perfectly. However, I am not pleased with the code. Basically I had a .txt file called lesson1.txt. I saved it as UTF-8 so it can contain Japanese characters.

I had the problem that I couldn't directly use the Japanese characters in JavaScript. I tried bypassing that issue by finding the Unicode code point of each character in PHP, echo that out in as a JavaScript array and use a JavaScript function to convert it back to the Japanese characters.

Now I figured this is not the way to go, because someone on this site told me this is an awful way of coding. My question is, how do I go by improving this? How can I get the Japanese characters into my JavaScript code the most efficient way? Also, have a look at how badly it is coded right now to get a better way of understanding my problem.

In case necessary: I used this PHP loop to get each Unicode point transferred into characters and saved into a JavaScript array:

$x = 0;
//$LineArray is an array in which each element contains a line of  lesson1.txt
    // so for example: $LineArray[0] = "あつい";
foreach($LineArray as $s)
{
    echo "TextCharacters[" . $x . "] = new Array();\n";
    for($i = 0; $i < mb_strlen($s,"utf-8"); $i++)
    {
        $char = mb_substr($s, $i, 1, "utf-8");
                    // REMOVE BOM
        if(strtoupper(bin2hex(iconv('UTF-8', 'UCS-2', $char))) != "FEFF") 
        {
            echo "TextCharacters[" . $x . "].push(go(\"" .   strtoupper(bin2hex(iconv('UTF-8', 'UCS-2', $char))) . "\"));\n";    
        }
    }
    $x++;
}
\$\endgroup\$
0
2
\$\begingroup\$

First off, post any and all code that you want reviewed. Just posting a little and saying the rest is "here" is not going to get you many good answers. Some of us dislike venturing to sites unknown to view someone else's code. If you post the rest of it and drop me a comment I will return to take a look at it if you wish. That being said, here is my take on your current code. Note: I reference the other two answers heavily, so this is merely an addition and not a true answer.

Incremental Variables

I don't really agree with Hermann's first comment. "x" is only a bad variable name if it is being used for something concrete and easily expressible in some other way. Here it is an incremental, a throw-away variable, so nothing is wrong with it. It's odd, "i" is usually used as the default incremental with "j" usually being secondary, but there's nothing wrong with "x". I do agree that it came out of no where and might be a little jarring, but if you move that line down to just above the loop it will be less so. Also, following jsanc623's example would also be acceptable.

For vs Foreach

The advantage to using a foreach loop over a for loop is that the first uses internal pointers that are dynamically updated, while the second uses external pointers that are statically updated. Another advantage is speed. A foreach loop is usually faster than a for loop. There are more advantages, but I'm going to stick with the ones that are important here. There is one instance where a for loop is better than a foreach loop. That is when you are looping over an associative array and using an external pointer inside of it that coincides with the internal pointer. So, instead of using a foreach loop in these circumstance, it may be best to use a for loop with array_keys() to access the array index manually. If, on the other hand, you are looping a numerically indexed array you can just use the key of the current element.

foreach( $LineArray AS $x => $s ) {

Variable Names

Again, I have to disagree with Hermann, there is nothing wrong with calling your variables an array, or a map, or anything else that indicates what they hold. Unnecessary? Sure, but at the same time it doesn't hurt anything and can actually help in some cases. Now, I won't say that calling a variable a "String" or an "Int" is good. PHP is very loosely typed, and that can easily change those contents, but adding an "array" or "obj" suffix to your variable lets those reading it know what it is supposed to be and is common enough.

I do agree with Hermann about $s however. It is a horrible name. I would expect to see $line or something similarly related to the array. The only single letter variables you should have are incremental (i, j) or something else that is commonly expressed with a single letter, such as coordinates (x, y, z).

Strings

I'm going to get argued with about this next suggestion, so let me just say up front that it is a stylistic choice that has some basis in fact. PHP allows for two methods of adding variables to a string. The first is escaping. Using double quotes to surround a variable or escape sequence you can escape the variable or sequence without any special syntax, just include them between the double quotes ("$var\n"). The second is concatenation and is the method you are currently using ('abc' . $var1 . "\n"). When concatenating it is best, in my opinion, to use single quotes instead of double. There's one major reason for this, double quotes tells PHP your string has entities that need escaping, whether they are PHP variables or manually escaped returns, tabs, etc... doesn't matter. So when you use double quotes PHP expects these escape routines and uses a small amount of extra processing power to parse the string for them. Now the processing power required and the time it takes are both negligible, but there is a difference and I like to point it out.

For Loops

Another thing about for loops. The arguments passed to a for or while loop are reparsed on each iteration. So passing a function in as an argument is usually a bad idea, at least if that function's return value is not meant to change i.e.(count() or in this case mb_strlen()). Both get the number of iterations the loop is going to go through. These numbers typically don't change between iterations so there is no need to recalculate it. It is just taking up extra processing power. If you move this function out of the argument list and set it as a variable it will run a bit faster, though it may be negligible in this case. jsanc623 touched on this, but I wanted to give the full reason.

Nested Functions

Again, I agree with Hermann, don't have long nested function calls in a statement. Declare it a variable before checking it, especially if you are going to be using it again elsewhere. This follows the DRY (Don't Repeat Yourself) Principle. Personally, I would break it up further, as I don't like nested function calls at all, but that might just be a stylistic choice. Here's how I'd write your if statement.

$bin = iconv('UTF-8', 'UCS-2', $char);
$hex = strtoupper( bin2hex( $bin ) );//Only one nested, and still not as desirable.
if( $hex != "FEFF" ) {

Functions

Again jsanc623 brings up a good point. This single function should be broken up into multiple functions. He doesn't come right out and say it, but it is implied. So I figured I'd go ahead and say it for him. Functions should do one thing and do it well. They should not be concerned with anything else. This is the Single Responsibility Principle.

Pushing to an Array

The following is equivalent to pushing onto the end of an array, and is usually the preferred method for appending array data.

TextCharacters[ 0 ] [] = go( '304A' );

Now, in your comment you said that you were pushing each element. Am I to assume you are doing so manually? If so, a better way would be to create an array of these hex values and loop over them to add them. Even better would be to create a JSON document, that way they could be accessed from both PHP and JS if needed.

\$\endgroup\$
2
  • \$\begingroup\$ Wow, you really took your time writing this. Its an old project of mine so sorry for posting late but you really did make sense about some stuff. Thanks for your time. \$\endgroup\$ Oct 14 '12 at 18:56
  • \$\begingroup\$ By the way, whats the difference between internal and external pointers? (in your for vs foreach paragraph) \$\endgroup\$ Oct 14 '12 at 19:06
0
\$\begingroup\$

Try this and see if you like it - made a few improvements (see comments) and created a couple of functions to reduce duplicate code and increase readability.

$counter = 0;

# Set Test Data
$currentLessonLine[0] = "あつい";
$currentLessonLine[1] = "あつい";
$currentLessonLine[2] = "あつい";
$currentLessonLine[3] = "あつい";
$currentLessonLine[4] = "あつい";

/**
 * $currentLessonLine is an array in which each element contains a line of  lesson1.txt
 * so for example: $currentLessonLine[0] = "あつい";
 */
foreach($currentLessonLine as $lessonLine){
    printOut($counter);

    /** 
     * Let's only find the string length once, shall we? Storing length in a 
     * variable instead of finding length every time is better practice.
     */
    $lessonLineLength = mb_strlen($lessonLine,"utf-8");

    for($i = 0; $i < $lessonLineLength; $i++){
        $char = mb_substr($lessonLine, $i, 1, "utf-8");

        # REMOVE BOM
        if(charConvert($char) != "FEFF") printOut($counter, TRUE, $char);
    }

    $counter++;
}


/**
 * if $selector == TRUE, $char must be set
 * function gets rid of print statements above and makes it cleaner
 */
function printOut($counter, $selector = FALSE, $char = ""){
    if($selector == TRUE && !empty($char)){
        echo "TextCharacters[" . $counter . "].push(go(\"" . charConvert($char) . "\"));\n";   
    } else {
        echo "TextCharacters[" . $counter . "] = new Array();\n";
    }
}

/**
 * You were duplicating this code twice above - once in the if-statement
 * and once in the if-body, this just takes reference to char and mods it 
 */ 
function charConvert(&$char){
    return strtoupper( bin2hex( iconv('UTF-8', 'UCS-2', $char) ) );
}
\$\endgroup\$
0
\$\begingroup\$

X is a very bad variable name. It comes like a lightning from a clear sky there in the beginning.

The following comment is good:

//$LineArray is an array in which each element contains a line of  lesson1.txt
// so for example: $LineArray[0] = "あつい";

But how about naming the array "current_lesson__lines"?

An array should never state in its name that it's an array. That's kinda like naming your buddy 'human john', just to make sure people realize that he's a human.

I think $cline_content would be better than $s.

In the following:

if(strtoupper(bin2hex(iconv('UTF-8', 'UCS-2', $char))) != "FEFF")

You're doing many things in a single session here.
That's always bad, try to do just one thing at a time.
Always doing only one thing at a time boosts readability, reuse-ability and modularity.
It is a formula for high quality.

\$\endgroup\$
1
  • \$\begingroup\$ Hello, Thanks for your help. I agree on your comment that the naming is a bit weak, but my real concern here is that the current structure of getting the japanese words into javascript is bad. The real question is: How can I get the japanese characters into my javascript code the most efficient way? If you look at my current site you'll see me having to push each character in a multidimensional array. Example: TextCharacters[0].push(go("304A")); TextCharacters[0].push(go("304A")); TextCharacters[0].push(go("304D")); TextCharacters[0].push(go("3044")); TextCharacters[0].push(go("000D")); \$\endgroup\$ Jul 20 '12 at 13:56

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.