patternjavascriptMinor
Korean Romanization to Hangeul library
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,
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;
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:
This would be magnitudes better:
Expected an assignment or function call and instead saw an expression
This is the warning given by JSHint for this piece of code:
This maybe subjective, but instead of
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
To make this valid, you could terminate the re-assignment of
In case you're wondering, you couldn't simply put
Unexpected use of '~'
Another warning by JSHint, for this bit:
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:
More 'clever' code
What's up with statements like this:
Why make things simple complicated?
You really should focus on making things as simple as possible:
Another thing, the Hungarian notation is frowned upon.
Just call it
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
Since the outer
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
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:
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:
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 = !1Why 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 (
!0is reallytrue, and!1is reallyfalse)
- 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.