Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

One of the things that's straight-forward in JavaScript is Unicode support. If you want to return ϝ (lowercase digamma, not a simple f), you simply tell it to do so. No look-up, conversion or unreadable Unicode codes required.

I want to build a translator between Arabic and Greek numerals. Since my projects tend to take forever to complete, I decided to post what I have instead of waiting till it's finished. The code currently supports 1 - 10 and translates them to the Unicode Greek equivalent.

I wrote it the most straight-forward way I could think of. It's not necessarily the best way.

function arabicToGreek(input) {
    'use strict';
    switch (input) {
    case 1:
        return "α";
    case 2:
        return "β";
    case 3:
        return "γ";
    case 4:
        return "δ";
    case 5:
        return "ε";
    case 6:
        return "ϝ";
    case 7:
        return "ζ";
    case 8:
        return "η";
    case 9:
        return "θ";
    case 10:
        return "ι";
    }
    return "";  
}

Usage:

arabicToGreek(4);    
arabicToGreek(10);

Output:

δ
ι

Run it at repl.it

I'm aware ϝ (6) should possibly be written as ϛ. I'm also aware I may or may not need to include diacritics.

I'm especially looking for a review on the usual suspects: naming, idiomatic style and modularity. All other advice is welcome as well.

share|improve this question
up vote 13 down vote accepted

It's great to see that you're using strict mode for strict compliance (which should really be the default), however you shouldn't use strict mode the way you are now. You've probably seen strict mode placed inside of functions, but those functions are IIFEs (Immediately Invoked Function Expressions) - it looks a bit like this.

(function() {
  'use strict'
}())

The reasoning behind why we encapsulate 'use strict' inside of a function is pretty simple: 'use strict' switches on strict compliance for all scripts in the same execution scope that the directive is executed in that are loaded after the directive.

In English, what that means is if you don't encapsulate the 'use strict' directive inside of a function, any script loaded after the script that initially uses 'use strict' will have strict compliance turned on as well. Some scripts (unfortunately) are not written with strict compliance in mind and will promptly throw their toys out of the pram if this happens.

So why do I not recommend putting it in arabicToGreek? Well, strict mode is a good thing (and should be standard), so it'd be better to just define it once per project. For larger projects that use things like Babel, strict mode is automatically applied; for smaller projects, you should just wrap your entire project in an IIFE that has the 'use strict' directive to prevent yourself from having to redefine it all of the time.

(function() {
  'use strict'

  function arabicToGreek(input) {
    ...
  }
}())

Note, if you're using Node, each module is already implicitly wrapped in an IIFE, so you can use 'use strict' in each file just fine.


You'd need to be an electrician to maintain that many switches! As suggested by Quill, you can use an object instead of a large switch statement, which is what I would do:

const characterMap = {
  1: 'α',
  2: 'β'
}

function arabicToGreek(code) {
  return characterMap[code]
}

Of course, this looks a bit like an array, so that could work too, but given that the index of the character is important here I would prefer to use an object just for clarity.

I don't agree with the idea of using hasOwnProperty here; if a character doesn't exist in the characterMap object, then attempting to access that property is just going to return undefined. JavaScript has another alternative for absent values called null.

The way I like to see the difference is this: undefined is a value that hasn't been defined; null is a value that has been defined as explicitly being absent. As a result, in this instance, I would check to see whether or not the expression characterMap[code] evaluates undefined and, if it does, return null instead.

function arabicToGreek(code) {
  const character = characterMap[code]
  if(character === undefined) {
    return null
  }
  return character
} 

If we abuse the fact that undefined, null, 0 and false all evaluate false in JavaScript (also known as "falsey"), this can be shortened using the logical OR operator (||) to look like this:

function arabicToGreek(code) {
  return characterMap[code] || null
}

Note that in all examples I've used above I've used const. const is a keyword introduced in ES6 that essentially ensures that the identifier can be assigned to once and only once. Attempting to reassign it will cause a runtime error.

If you're running this code in an ES5 browser that doesn't support ES6, you'll run into issues because (ironically), 'use strict' breaks const prior to ES6 - in ES5, const was a reserved keyword that didn't actually do anything - same thing as class. In ES6 these were both codified into actual concepts in the spec. They should work fine on any reasonably modern browser, though, and in any of the newer versions of Node.

As an alternative to using an Object, you may be interested in the ES6 Map, which is a fairly close analogue to a Dictionary from C# or a Map from Java. Key benefits include Map not having a prototype (no need to do hasOwnProperty), keys can be any value (keys in objects are implicitly converted to a string - or Symbol), and retrieving the length of a Map is actually possible without sacrificing your soul to Zalgo.

share|improve this answer
3  
I find your lack of semicolons disturbing... – Kroltan Jan 16 at 16:20
    
@Kroltan haha, I prefer to not use them :P – Dan Pantry Jan 16 at 16:29
    
Is "I prefer to not use them" only a joke? If not, I'm curious and interested to know your reasons. – cFreed Jan 16 at 18:59
1  
Thanks for the interesting link. But it enforces my astonishment: it seems to me that the concern to avoid errors (and BTW increase readability) would lead to prefer using semicolons systematically. I find this type of choice is equivalent to the one about using blocks where they are not mandatory with if(), for() and so on. But actually, sure it"s a stylistic choice, as you stated. – cFreed Jan 16 at 20:26
1  
@cFreed I figure that the cases where it causes issues are so rare (as long as you code in a normal manner) that it doesn't really matter; consistency is the key either way. – Dan Pantry Jan 16 at 20:27

Reviewing your current structure: your return ""; could be improved: it should / could be inside the switch statement as a default case.


It's better to use an object here, as later you can extend it without having to spend pointless LoCs on cases and breaking.

function arabicToGreek(input) {
    'use strict';
    var greek = {
        1: "α",
        2: "β",
        3: "γ",
        4: "δ",
        5: "ε",
        6: "ϝ",
        7: "ζ",
        8: "η",
        9: "θ",
        10: "ι"
    };
    //
}

Then, add greek.hasOwnProperty to check that your string is a valid object property, in which case that it isn't, you can throw an error.

if (!greek.hasOwnProperty(input)){
    throw new Error("input not a valid integer representing a character");
}

This also means you can store the greek characters and potentially other languages in an external JSON document.

share|improve this answer
    
This would definitely solve part of the maintainability issues. Thanks :-) – Mast Jan 15 at 13:42
5  
'use strict' should not be inside of arabicToGreek. If you want to restrict what is 'use strict', then wrap the entire code file in an IIFE. You run the risk of weird compatibility errors between functions if some functions are strict-mode and others are not. I wouldn't throw in the event of an unknown character; I would instead return null to indicate the absence of a corresponding value. – Dan Pantry Jan 15 at 14:22

That looks like a classic case for a lookup table. This function should be a one-liner.

We know that parameters are inputs. What kind of input? Also, the input isn't Arabic; it's just abstractly a number. It would be better to rename the parameter and function to number.

function numberToGreek(number) {
    return "αβγδεϝζηθι".charAt(number - 1);
}
share|improve this answer
    
That's definitely the most straight-forward way to do it. An eye-opener, I hadn't considered doing it like this before. – Mast Jan 15 at 23:46
1  
This is a cool idea, but the reason I didn't suggest it is because you @Mast may not have wanted to have the entire map rely on implicit ordering. – Dan Pantry Jan 16 at 12:09
    
@DanPantry Now both options are explained I can figure out what fits best. Thanks to you both. – Mast Jan 16 at 12:19
    
Be wary when using native JS methods on strings that may contain unicode - some characters (e.g. those above U+FFFF) are stored and interpreted as two 16-bit characters, giving misleading results. – Robert K. Bell Jan 20 at 1:24

I wouldn't use anything other than a string to express this - in general sequential translations could be encoded as string index lookup

function arabicToGreek(input) {
    return " αβγδεϝζηθι"[input];
}

Or in ES2015:

let arabicToText => i => " αβγδεϝζηθι"[i];
share|improve this answer
2  
The difference is that your arabicToGreek(0) returns a Space, whereas mine returns an empty string like the original code. – 200_success Jan 16 at 2:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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