1
\$\begingroup\$

Any suggestions for improvement or critiques are welcome!

<!DOCTYPE html>
<html>
<body>


<canvas id="myCanvas" width="400" height="200" style="border:1px solid #d3d3d3;">
Your browser does not support the HTML5 canvas tag.</canvas>
<input type="button" onclick="alert(ColorToText())" value="Click Here"/>
<script>
function TextToColor(arg){
    var c = document.getElementById("myCanvas");
    var ctx = c.getContext("2d");
    var text = arg;
    var color;
    var i;
    for(i = 0;i<text.length;i++)
    {
        switch(text.charAt(i)) 
        {
        case "a": 
            color = "#000020";
            break;
        case "b": 
            color = "#000040";
            break;
        case "c": 
            color = "#000060";
            break;
        case "d": 
            color = "#000080";
            break;
        case "e": 
            color = "#0000a0";
            break;
        case "f": 
            color = "#0000c0";
            break;
        case "g": 
            color = "#0000e0";
            break;
        case "h": 
            color = "#001010";
            break;
        case "i": 
            color = "#002030";
            break;
        case "j": 
            color = "#003050";
            break;
        case "k": 
            color = "#004070";
            break;
        case "l": 
            color = "#005090";
            break;
        case "m": 
            color = "#0060b0";
            break;
        case "n": 
            color = "#0070d0";
            break;
        case "o": 
            color = "#0080f0";
            break;
        case "p": 
            color = "#009020";
            break;
        case "q": 
            color = "#00a040";
            break;
        case "r": 
            color = "#00b060";
            break;
        case "s": 
            color = "#00c080";
            break;
        case "t": 
            color = "#00d0a0";
            break;
        case "u": 
            color = "#00e0c0";
            break;
        case "v": 
            color = "#00f0e0";
            break;
        case "w": 
            color = "#102010";
            break;
        case "x": 
            color = "#204030";
            break;
        case "y": 
            color = "#306050";
            break;
        case "z": 
            color = "#408070";
            break;
                case " ":
            color = "#50a090";
            break;
        case "-":
            color = "#60c0b0";
            break;
        case "=":
            color = "#70e0d0";
            break;
        case "0":
            color = "#800000";
            break;
        case "1":
            color = "#801000";
            break;
        case "2":
            color = "#802000";
            break;
        case "3":
            color = "#803000";
            break;
        case "4":
            color = "#804000";
            break;
        case "5":
            color = "#805000";
            break;
        case "6":
            color = "#806000";
            break;
        case "7":
            color = "#807000";
            break;
        case "8":
            color = "#808000";
            break;
        case "9":
            color = "#809000";
            break;
        default:
            color = "#000000";
            break;
        };
        ctx.fillStyle = color;
        ctx.fillRect(i*1, 0, 1, 1);
    };
}
function componentToHex(c) {
    var hex = c.toString(16);
    return hex.length == 1 ? "0" + hex : hex;
}

function rgbToHex(r, g, b) {
    return "#" + componentToHex(r) + componentToHex(g) + componentToHex(b);
}
function ColorToText() 
{
    var text = "";
    var c = document.getElementById("myCanvas");
    var ctx = c.getContext("2d");
    var imgData=ctx.getImageData(0,0,c.width,c.height);
    for(var i = 0;i<imgData.data.length;i+=4)
    {
    var imgData=ctx.getImageData((i*1)/4,0,10,10);
    red = imgData.data[0];
    green = imgData.data[1];
    blue = imgData.data[2];         
    var hex = rgbToHex(red, green, blue);
    switch(hex)
    {
        case "#000020":
            text+= "a";
            break;
        case "#000040":
            text+= "b";
            break;
        case "#000060":
            text+= "c";
            break;
        case "#000080":
            text+= "d";
            break;
        case "#0000a0":
            text+= "e";
            break;
        case "#0000c0":
            text+= "f";
            break;
        case "#0000e0":
            text+= "g";
            break;
        case "#001010":
            text+= "h";
            break;
        case "#002030":
            text+= "i";
            break;
        case "#003050":
            text+= "j";
            break;
        case "#004070":
            text+= "k";
            break;
        case "#005090":
            text+= "l";
            break;
        case "#0060b0":
            text+= "m";
            break;
        case "#0070d0":
            text+= "n";
            break;
        case "#0080f0":
            text+= "o";
            break;
        case "#009020":
            text+= "p";
            break;
        case "#00a040":
            text+= "q";
            break;
        case "#00b060":
            text+= "r";
            break;
        case "#00c080":
            text+= "s";
            break;
        case "#00d0a0":
            text+= "t";
            break;
        case "#00e0c0":
            text+= "u";
            break;
        case "#00f0e0":
            text+= "v";
            break;
        case "#102010":
            text+= "w";
            break;
        case "#204030":
            text+= "x";
            break;
        case "#306050":
            text+= "y";
            break;
        case "#408070":
            text+= "z";
            break;
        case "#50a090":
            text+= " ";
            break;
        case "#60c0b0":
            text+= "-";
            break;
        case "#70e0d0":
            text+= "=";
            break;
        case "#800000":
            text+= "0";
            break;
        case "#801000":
            text+= "1";
            break;
        case "#802000":
            text+= "2";
            break;
        case "#803000":
            text+= "3";
            break;
        case "#804000":
            text+= "4";
            break;
        case "#805000":
            text+= "5";
            break;
        case "#806000":
            text+= "6";
            break;
        case "#807000":
            text+= "7";
            break;
        case "#808000":
            text+= "8";
            break;
        case "#809000":
            text+= "9";
            break;
        };
    };
    return text;
}
TextToColor(("0663089000004 EVO-RC EVOLUTION CANDLE REMOTE 00663089005009").toLowerCase());

</script>

</body>
</html>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You should leave your original code unaltered. See For an iterative review, is it okay to edit my own question to include revised code? From the help center, "...we ask you to please refrain from modifying the original code based on what has been addressed in answers. By modifying the code in such a way, it'll be harder to determine the merit of the answers, as well as easily seeing the original code that was reviewed." \$\endgroup\$
    – apsillers
    Commented Jan 16, 2015 at 23:58

3 Answers 3

Reset to default
3
\$\begingroup\$

A massively simpler approach to your switch/case would be to use an object:

var charToHexMapping = {
   "a":"#000020",
   "b":"#000040",
   ...
}

Then, read from that object with:

var i, char;
for(i = 0;i<text.length;i++)
{
    char = text.charAt(i)
    if(charToHexMapping.hasOwnProperty(char))
    {
        color = charToHexMapping[char];
    }
    else
    {
        color = "#000000";
    }
}

You'd need another object to do the reverse transformation, though, so it might be better to define a list of pairs and build each object programatically:

var charHexPairs = [
   ["a", "#000020"],
   ["b", "#000040"],
   ...
]

var charToHexMapping = {},
    hexToCharMapping = {};

var i, pair, char, hex;
for(i=0; i < charHexPairs.length; i++)
{
    pair = charHexPairs[i];
    char = pair[0];
    hex = pair[1];
    charToHexMapping[char] = hex;
    hexToCharMapping[hex] = char;
}

Then use the charToHexMapping object as above (and the hexToCharMapping similarly in the reversal).

Other notes:

  • red, green, and blue in ColorToText are missing var declarations and so are implicit globals.
  • Function names should start with a lowercase letter (e.g., textToColor). An initial uppercase letter conventionally indicates a constructor.

  • The inline onclick listener should be made into an event listener instead:

    <input type="button" id="gobutton" value="Click Here"/>
<script>
    document.getElementById("gobutton").addEventListener(function() {
        alert(ColorToText())
    });

  • To make your code more modular, you might consider encapsulating it all in an object that takes a canvas argument as a constructor:

    var ColorCoder = function(canvas) {
    this.canvas = canvas;
}

ColorCoder.prototype.textToColor = function(arg) {
    var ctx = this.canvas.getContext("2d");
    ...
}

var coder = new ColorCoder(document.getElementById("myCanvas"));
coder.textToColor(("0663089000004 EVO-RC EVOLUTION CANDLE REMOTE 00663089005009").toLowerCase());
\$\endgroup\$
1
  • \$\begingroup\$ Another excellent suggestion to define in pairs and build each table in a more programmatic fashion. \$\endgroup\$
    – sdin3dspace
    Commented Jan 16, 2015 at 20:27
1
\$\begingroup\$

Return instead of breaking

Your ColorToText function looks like this right now:

var text = "";

switch(hex)
{
    case "#000020":
        text+= "a";
        break;

[...]
    case "#809000":
        text+= "9";
        break;
    };
};
return text;

This leaves the false impression, that you are adding up the value of text. If you just return the value, your code will also be shorter:

switch(hex)
{
    case "#000020":
        return "a";
    // [...]
    case "#809000":
        return "9";
    };
};

Avoid duplication

Right now, you have the mapping between colors and text twice, each in a very long switch statement. This duplication makes it hard to find bugs (eg a mapped to #000020, but #000020 mapped to b), and it also makes it hard to add additional colors/text, as your code needs to be edited in two places.

Instead, you could use an array to look up these values. It might look something like this:

var colorTextMap = {
  "#000020" : "a",
  // [...]
  "#809000" : "9",
};

// color to text:
var color = "#000020";
alert(colorTextMap[color]); 

// text to color
var text = "a";
for(var color in colorTextMap) {
    if (colorTextMap[color] == text) {
        alert(color); 
    }
}

This approach should reduce the size of your code by quite a bit, and make it easier to maintain as well.

Misc

  • use more spaces to increase readability (after , and ;, around <, *, etc).
  • function names should start with a lowercase letter
  • be consistent with your placement of opening curly brackets (in JavaScript, the common placement seems to be the same line as the opening statement).
  • write out your variable names. right now, it might seem obvious that c stands for color or canvas, but will this still be true if you look at this code in a month?
  • be consistent with your variable names. either it's r, g, b, or red, green, blue (in this case I think either is fine, as r, g, b are commonly in use).
\$\endgroup\$
1
  • \$\begingroup\$ Excellent recommendations, thank you!! I will improve it and try include more default flow information in the code itself. \$\endgroup\$
    – sdin3dspace
    Commented Jan 16, 2015 at 19:29
0
\$\begingroup\$

In the previous textToColor code I wrote, I did not account for the canvas width when writing pixels and just used the index to write the next pixel to the canvas.

Improved writer to wrap around the border of a image. on a 200 x 200 image there is 200 pixels top to write to. 199 pixels on the right, 199 pixels on the left, and finally 198 pixels on the bottom totaling 796 pixels.

Add wraparound writing to canvas.

ColorizeData.prototype.textToColor = function(arg){
    var context = this.canvas.getContext("2d");
    context.clearRect(0, 0, this.canvas.width, this.canvas.height);
    var index;
    var upDown = 1;
    var upDown1 = 1;
    var leftRight = 0;
    arg = arg.toLowerCase();
    context.globalAlpha = 1;
    for(index = 0;index<arg.length;index++)
    {
        var prop = arg.charAt(index);
        if(this.charToHexTable.hasOwnProperty(prop))
        {
            context.fillStyle = this.charToHexTable[prop] ; 
        }
        else
        {
            context.fillStyle = "#000000";
        }
        if(index < this.canvas.width) {
            context.fillRect(index, 0, 1, 1);
            leftRight = index;
        }
        else
        {   
            if(upDown < this.canvas.height) {
                context.fillRect(leftRight,upDown,1,1);
                upDown += 1;
            }
            else
            {
                if(upDown1 < this.canvas.height) {
                    leftRight = 0
                    context.fillRect(leftRight,upDown1,1,1);
                    upDown1 +=1
                }
                else
                {
                    if(leftRight < this.canvas.width -2) {
                        leftRight+= 1;
                        context.fillRect(leftRight,upDown1-1,1,1);
                    }
                }
            }
        }
    };
}

Add wraparound reading for data bigger than the width of the canvas. Greatly improved reader speed by properly handling the data returned by getImageData and followed same format of writer to read data top right left bottom.

ColorizeData.prototype.colorToText = function(arg) 
{
    var text = "";
    var context = this.canvas.getContext("2d");
    var imgData = context.getImageData(0,0,this.canvas.width,this.canvas.height);
    var index;
    var size = 0;
    for(index = 0;index<4;index+= 1)
    {
        if(index == 0) {
            imgData = context.getImageData(0,0, this.canvas.width,1);
            size = this.canvas.width;
        }
        else
        {
            if(index == 1) {
                imgData = context.getImageData(size -1,1, 1,this.canvas.height - 1);
                size = this.canvas.height - 1;  
            }
            else
            {
                if(index == 2) {
                    imgData = context.getImageData(0,1, 1,this.canvas.height - 1);
                    size = this.canvas.height - 1;
                }
                else
                {   
                    imgData = context.getImageData(1,this.canvas.height -1, this.canvas.width-2, 1);
                    size = this.canvas.width-2; 
                }
            }
        }
        for( var i = 0;i < size*4;i+=4) {
            var r = imgData.data[i+0];
            var g = imgData.data[i+1];
            var b = imgData.data[i+2];         
            var hex = rgbToHex(r, g, b);
            if(this.hexToCharTable.hasOwnProperty(hex))
            {
                text += this.hexToCharTable[hex];
            }
        };

    };
    document.getElementById(arg).innerText = text;
}
\$\endgroup\$
0

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.