7
\$\begingroup\$

After a little work, I've finally ironed this out... Though could I get someone to go over this and see if there is a better way to present this library within the code, before I publish it?

Premise: Kimchi (a Korean transliteration library)

Kimchi, apart from being a delicious Korean side dish is also a Korean transliteration library that takes romanized input and transliterates (not translates) said text into the equivalent Hangeul character syllable blocks.

There are currently two commands, kimchi.all() and kimchi.convert(str); kimchi.convert(str) will convert the str string variable into the corresponding Korean syllable blocks. kimchi.all() will go through the DOM and apply kimchi.convert() on the innerHTML component of each <kimchi> tag. Basically treat <kimchi> tags the same way as you would a <span> tag.

The library (save as kimchi.js):

var kimchi = {
    convert: function(fromText) {
        // Internal search function, avoids issues with non existent Array.indexOf() functionality in certain versions
        var inArr=function(a,s){for(var x=-1,y=0;y<a.length;y++)if(a[y]==s){x=y;break}return x};

        // Lower case the input text, build LVT arrays and init output text
        fromText = fromText.toLowerCase(),
        aLead = "g0kk0n0d0tt0r0m0b0pp0s0ss0-0j0jj0ch0k0t0p0h".split(0),
        aVowel = "a0ae0ya0yae0eo0e0yeo0ye0o0wa0wae0oe0yo0u0wo0we0wi0yu0eu0ui0i".split(0),
        aTail = "g0gg0gs0n0nj0nh0d0l0lg0lm0lb0ls0lt0lp0lh0m0b0bs0s0ss0ng0j0c0k0t0p0h".split(0),
        toText = "";

        // Split into Alpha/Dash and non-Alpha/Dash arrays
        // Then it's simply a matter of flipping between the two arrays and displaying accordingly.
        aKorean = fromText.split(/[^a-z\-]+/) || [],
            aNotKorean = fromText.split(/[a-z\-]+/) || [],
            iType = 3,
            iPhase = 0;
        // Loop through each Korean word.
        for (var iLoop = 0; iLoop < aKorean.length; iLoop++) {
            sWord = aKorean[iLoop];

            // Determine if first letter is vowel, if so latch a dash on its front.
            sWord = (~"aeiouwy".indexOf(sWord[0]) ? "-" : "") + sWord;

            // Loop through in reverse and extract letter blocks.
            for (var sLetterExt = "", bFoundLetter = !1, sLetterBlock = "", iLoop2 = sWord.length - 1; - 1 < iLoop2; iLoop2--)
                                for (var iLocationLast = iLoop2, iLen = 1; !bFoundLetter;) {
                                        // Attempt to find letter in chosen array
                                        // This is important, as there is no chance that a letter will be missed... 
                                        // At least I hope not...
                                    sExtract = sWord.substr(iLocationLast - iLen + 1, iLen);
                                    iLocation = inArr([aLead, aVowel, aTail][iType - 1], sExtract);
                                    if (0 > iLocation && "" == sLetterExt) {
                            // If the letter isn't found, move to next logical array,
                            // move cursor back and break out of current loop for restart.
                                        iType = 1 > iType - 1 ? 3 : iType - 1;
                                        iLoop2++;
                                        break
                                    }
                                    if (0 > iLocation && "" == sLetterExt && 1 == iPhase) {
                            // If the letter found is a - and the previous letter found is a lead,
                            // ignore it as this is plainly a divisionary - and not a mute.
                            // Move to tail array, move cursor forward and break out of 
                            // current loop for restart.
                                        iType = 3;
                                        break
                                    } else if (0 > iLocation && "" != sLetterExt && 0 == iLocationLast && 1 == iLen && 1 == iType) {
                        // It's gone as far as it can, and it can't go any further...
                        // Flag letter found and assign letter type, then relocate cursor.
                                        bLetterFound = !0;
                                        sLetterBlock = (1 == iType ? " " : "") + (1 < iType ? "/" : "") + sLetterExt + sLetterBlock;
                                        iLoop2 = iLocationLast - iLen + 1;
                                        sLetterExt = "";
                                        break
                                    } else if (0 > iLocation && "" != sLetterExt)
                            // It's gone as far as it can, and it can't go any further...
                            // Flag letter found and assign letter type, then relocate cursor.
                                            bLetterFound = !0, 
                                            iType == iPhase && "" != sLetterBlock && (sLetterBlock = " -" + sLetterBlock), 
                                            sLetterBlock = (1 == iType ? " " : "") + (1 < iType ? "/" : "") + sLetterExt + sLetterBlock, 
                                            iLoop2 = iLocationLast - iLen + 1, 
                                            sLetterExt = "", 
                                            iPhase = iType;
                                    else if (sLetterExt = sExtract, 0 == iLocationLast) {
                                        bLetterFound = !0;
                                        sLetterBlock = sExtract + sLetterBlock;
                                        iType = 3;
                                        break
                                    } else iLen++
                                }

                        sKoreanConv = "";
                        aConvKorean = sLetterBlock.split(" ");
                        // Determine the Unicode character value for the syllable block... 
                        for (var a = 0; a < aConvKorean.length; a++) 
                                "" != aConvKorean[a] && (aSyllable = aConvKorean[a].split("/"), sKoreanConv += "&#" + ((2 == aSyllable.length ? 0 : inArr(aTail, aSyllable[2]) + 1) + 28 * inArr(aVowel, aSyllable[1]) + 588 * inArr(aLead, aSyllable[0]) + 44032) + ";");
                        aKorean[iLoop] = sKoreanConv;
                        sKoreanConv = "";
        }
        bNotKoreanStart = aNotKorean ? 0 == fromText.indexOf(aNotKorean[0]) ? !0 : !1 : !1;
        iCounter = 0;
                if (bNotKoreanStart && aNotKorean.length == aKorean.length)
                    for (var iDisp = 0; iDisp < aNotKorean.length; iDisp++) toText += aNotKorean[iDisp] + aKorean[iDisp];
                else if (!bNotKoreanStart && aNotKorean.length == aKorean.length)
                    for (iDisp = 0; iDisp < aNotKorean.length; iDisp++) toText += aKorean[iDisp] + aNotKorean[iDisp];
                else if (bNotKoreanStart) {
                    if (toText += aNotKorean[0], 1 < aNotKorean.length)
                        for (iDisp = 0; iDisp < aKorean.length; iDisp++) toText += aKorean[iDisp] + aNotKorean[iDisp + 1]
                } else if (!bNotKoreanStart && (toText += aKorean[0], 1 < aKorean.length))
                    for (iDisp = 0; iDisp < aNotKorean.length; iDisp++) toText += aNotKorean[iDisp] + aKorean[iDisp + 1];
        return toText;
    },
    all: function() {
            // Affects all <kimchi> tags... 
        var z = document.getElementsByTagName('kimchi');
        for (x = z.length; x--;) {
            z[x].innerHTML = kimchi.convert(z[x].childNodes[0].data);
        }
    }
}

Usage:

<html>
    <head>
        <meta charset="utf-8" />
    </head>
    <script src="kimchi.js"></script>
    <body onLoad="kimchi.all();">
        <kimchi>hangug-eo, radio, seoul, pyeong-yang, kimchi</kimchi> - some random words in Korean<br/>
        <kimchi>Kim Yu Suk</kimchi>: the name of a genuine Korean Olympic high jumper<br />
        <span id="test"></span>
        <script>
            document.getElementById("test").innerHTML = kimchi.convert("pyeong-yang");
        </script>
    </body>
</html>
\$\endgroup\$
0

1 Answer 1

3
+50
\$\begingroup\$

Readability

As a library author, I think readability should be one of your top concerns. This is hardly considered readable, in fact it kinda breaks almost all known good practices of readable coding:

var inArr=function(a,s){for(var x=-1,y=0;y<a.length;y++)if(a[y]==s){x=y;break}return x};

This would be magnitudes better:

function in_arr(arr, item) {
    for (var index = 0; index < arr.length; index++) {
        if (arr[index] == item) {
            return index;
        }
    }
    return -1;
}

Expected an assignment or function call and instead saw an expression

This is the warning given by JSHint for this piece of code:

fromText = fromText.toLowerCase(),
aLead = "g0kk0n0d0tt0r0m0b0pp0s0ss0-0j0jj0ch0k0t0p0h".split(0),
aVowel = "a0ae0ya0yae0eo0e0yeo0ye0o0wa0wae0oe0yo0u0wo0we0wi0yu0eu0ui0i".split(0),
aTail = "g0gg0gs0n0nj0nh0d0l0lg0lm0lb0ls0lt0lp0lh0m0b0bs0s0ss0ng0j0c0k0t0p0h".split(0),
toText = "";

This maybe subjective, but instead of 0 as the separator symbol, I'd find space much much more readable.

Separating variable assignments with comma is used when declaring and assigning multiple variables in a single statement starting with the var keyword. To make this valid, you could terminate the re-assignment of fromText, and put var in front of aLead, like this:

fromText = fromText.toLowerCase();
var aLead = "g0kk0n0d0tt0r0m0b0pp0s0ss0-0j0jj0ch0k0t0p0h".split(0),
aVowel = "a0ae0ya0yae0eo0e0yeo0ye0o0wa0wae0oe0yo0u0wo0we0wi0yu0eu0ui0i".split(0),
aTail = "g0gg0gs0n0nj0nh0d0l0lg0lm0lb0ls0lt0lp0lh0m0b0bs0s0ss0ng0j0c0k0t0p0h".split(0),
toText = "";

In case you're wondering, you couldn't simply put var in front of fromText, because the variable is already defined.

Unexpected use of '~'

Another warning by JSHint, for this bit:

sWord = (~"aeiouwy".indexOf(sWord[0]) ? "-" : "") + sWord;

This is too clever. Clever code is not a virtue. It makes code harder to read. You could rewrite it less clever and without warnings:

sWord = ("aeiouwy".indexOf(sWord[0]) >= 0 ? "-" : "") + sWord;

More 'clever' code

What's up with statements like this:

bFoundLetter = !1

Why make things simple complicated? You really should focus on making things as simple as possible:

bFoundLetter = false;

Another thing, the Hungarian notation is frowned upon. Just call it foundLetter, simple, natural.

More warnings on JSHint

There are many many other warnings pointed out by JSHint, I suggest to copy-paste your code, review all reported violations, and try to fix them all.

Strange indentation

The second for statement here has crazy excessive indentation:

for (var sLetterExt = "", bFoundLetter = !1, sLetterBlock = "", iLoop2 = sWord.length - 1; - 1 < iLoop2; iLoop2--)
                    for (var iLocationLast = iLoop2, iLen = 1; !bFoundLetter;) {

Since the outer for statement is already quite deeply indented, this really forces the reader to scroll horizontally, which really seriously hurts readability. And it's completely unnecessary, you could just indent by 4 spaces like you do in most of the rest of the code.

Use braces always

A lot of for loops and if conditions don't use braces around their statement, for example:

if (bNotKoreanStart && aNotKorean.length == aKorean.length)
    for (var iDisp = 0; iDisp < aNotKorean.length; iDisp++) toText += aNotKorean[iDisp] + aKorean[iDisp];
else if (!bNotKoreanStart && aNotKorean.length == aKorean.length)
    for (iDisp = 0; iDisp < aNotKorean.length; iDisp++) toText += aKorean[iDisp] + aNotKorean[iDisp];

This goes against the common recommendation to always use braces. Especially in code that's already very hard to read, it's very important to diligently put those braces:

if (bNotKoreanStart && aNotKorean.length == aKorean.length) {
    for (var iDisp = 0; iDisp < aNotKorean.length; iDisp++) {
        toText += aNotKorean[iDisp] + aKorean[iDisp];
    }
} else if (!bNotKoreanStart && aNotKorean.length == aKorean.length) {
    for (iDisp = 0; iDisp < aNotKorean.length; iDisp++) {
        toText += aKorean[iDisp] + aNotKorean[iDisp];
    }
}

Not using braces always can cause grave problems like this famous bug.

Conclusion

The things I pointed out above are but a few examples of problems that appear in many places of the code. I suggest to:

  • Apply the above suggestions to all similar patterns in the code
  • Replace all "clever" tricks with simple their corresponding simple intuitive alternatives (!0 is really true, and !1 is really false)
  • Format the code nicely, make it as easy to read as possible
  • Go over all JSHint warnings and make your code as spotless as possible
\$\endgroup\$
3
  • \$\begingroup\$ Thank you! This is the kind of information I needed, and not down votes without comment... I believe I may have a simplified version somewhere. I'll replace the above with it shortly. \$\endgroup\$ Commented Jun 14, 2015 at 20:38
  • \$\begingroup\$ Don't change the code in your post though. See this relevant meta post. Btw there are no downvotes on your question now \$\endgroup\$
    – janos
    Commented Jun 14, 2015 at 20:39
  • \$\begingroup\$ Ah yes, the "selfie answer" I think they called it... I'll do it that way... \$\endgroup\$ Commented Jun 14, 2015 at 20:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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