HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Korean Romanization to Hangeul library

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
romanizationhangeullibrarykorean

Problem

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 ` tag. Basically treat tags the same way as you would a 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 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;

Solution

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

Code Snippets

var inArr=function(a,s){for(var x=-1,y=0;y<a.length;y++)if(a[y]==s){x=y;break}return x};
function in_arr(arr, item) {
    for (var index = 0; index < arr.length; index++) {
        if (arr[index] == item) {
            return index;
        }
    }
    return -1;
}
fromText = fromText.toLowerCase(),
aLead = "g0kk0n0d0tt0r0m0b0pp0s0ss0-0j0jj0ch0k0t0p0h".split(0),
aVowel = "a0ae0ya0yae0eo0e0yeo0ye0o0wa0wae0oe0yo0u0wo0we0wi0yu0eu0ui0i".split(0),
aTail = "g0gg0gs0n0nj0nh0d0l0lg0lm0lb0ls0lt0lp0lh0m0b0bs0s0ss0ng0j0c0k0t0p0h".split(0),
toText = "";
fromText = fromText.toLowerCase();
var aLead = "g0kk0n0d0tt0r0m0b0pp0s0ss0-0j0jj0ch0k0t0p0h".split(0),
aVowel = "a0ae0ya0yae0eo0e0yeo0ye0o0wa0wae0oe0yo0u0wo0we0wi0yu0eu0ui0i".split(0),
aTail = "g0gg0gs0n0nj0nh0d0l0lg0lm0lb0ls0lt0lp0lh0m0b0bs0s0ss0ng0j0c0k0t0p0h".split(0),
toText = "";
sWord = (~"aeiouwy".indexOf(sWord[0]) ? "-" : "") + sWord;

Context

StackExchange Code Review Q#93075, answer score: 3

Revisions (0)

No revisions yet.