0
\$\begingroup\$

I'm working on a language called grond (see it on GitHub); the compiler is so small it is included in this post.

My gut tells me that this code is awful, but it works. I am hesitant to change things to make the code better because generally, I have found that bad things happen when I "fix" something that isn't necessarily broken.

How can I make this "ugly" code better? Note - If you don't think this code is ugly, please tell me!

String.prototype.reverse = function() {
  return this.split("").reverse().join("");
}

function replace(wc) {
  var lastWC;

  while (true) {
    lastWC = wc;
    wc = wc.reverse();
    wc = wc.replace("§", "");
    wc = wc.replace("Ś", "");
    wc = wc.replace(/\(a(?!\w)/, "(trela"); //alert ,a
    wc = wc.replace(/\(f(?!\w)/, "(rof"); // for, f
    wc = wc.replace(/\(w(?!\w)/, "(elihw"); //while,w
    wc = wc.replace(/\(p(?!\w)/, "(tpmorp"); //prompt,p
    wc = wc.replace(/\(vr\./, "(esrever."); //reverse, rv
    wc = wc.replace(/\(tm\./, "(chtam."); // match, mt
    wc = wc.replace(/\(nj\./, "(nioj."); //join, .jn
    wc = wc.replace(/\(pr\./, "(ecalper."); //replace, .rp
    wc = wc.replace(/\(ls\./, "(.ecils"); //slice, sl
    wc = wc.replace(/\(lt\./, "(esaCrewoLot."); //toLowerCase, tl
    wc = wc.replace(/\(ut\./, "(esaCreppUot."); //toUpperCase, tu
    wc = wc.replace(/\(ps\./, "(tilps."); //split, sp
    wc = wc.reverse();
    wc = wc.replace(/\.l(?!\w)/, ".length"); // .length, .l
    if (lastWC === wc)
      break;
  }
  return wc;
}

var source = prompt("Enter Source");
var sourceCopy = source;


var fullASCII = ' !#$%&()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~'.split('');
var y = (a) => {
  for (let char in fullASCII) {
    if (!source.contains(char)) {
      source.replace(a, char);
    }
  }
}
var OK = false;
var OK2 = false;
for (let char in fullASCII) {
  if (!source.contains(char)) {
    OK2 = OK ? true : false;
    OK = true;
  }
}
if (!OK2) {
  throw "Your program uses every ASCII charactor. Grond requires your program to leave 2 ASCII chars unused. This is stupid and will be fixed.";
}
y('\\\"');
y("\\\'");
var sourceSplit = sourceCopy.split("\'");
for (let i = 0; i < sourceSplit.length; i += 2) {
  var o = sourceSplit[i].split('\"');
  for (let e = 0; e < o.length; e += 2) {
    o[e] = replace(o[e]);
  }
  sourceSplit[i] = o.join('\"');
}
sourceCopy = sourceSplit.join("\'");

var append = "if(s != \"\"){alert(s)}";
var prepend = "var s = prompt();";
var oSS = source.split('');
if (oSS[0] == "§") {
  prepend = "var s = \"\"";
}
if (oSS[0] == "Ś" || oSS[1] == "Ś") {
  prepend = "with(math){" + prepend;
  append += "}";
}

sourceCopy = prepend + " String.prototype.la=function(h){var x=this.split(''); x[h] = x[h].toLowerCase(); return x.join('');};String.prototype.ca=function(h){var x=this.split(''); x[h] = x[h].toUpperCase(); return x.join('');};String.prototype.reverse=function(){return this.split('').reverse().join('');}; \n var h = ['hello, world', 'Hello, World', 'Hello, World!', 'hello, world!', 'hello world', 'hello world!', 'Hello World', 'Hello World!', 'Hello, world.', 'Hello, World.' ,'hello world.' , 'Hello World.'];  var q ='" + source + "'; " + sourceCopy + append;
alert(sourceCopy);
eval(sourceCopy);

\$\endgroup\$
6
  • \$\begingroup\$ yes, the code works as far as I have tested it...thanks for the feed back (and not the downvote... I assume that was you) \$\endgroup\$ Apr 8, 2016 at 20:49
  • \$\begingroup\$ prompt();... you should click run code snippet or read the code... \$\endgroup\$ Apr 8, 2016 at 20:52
  • \$\begingroup\$ I have made the edits on github, is it customary on code review to edit the post with the edits you suggested as well? \$\endgroup\$ Apr 8, 2016 at 20:54
  • \$\begingroup\$ As long as there is no answer, feel free to update the question as you'd like. \$\endgroup\$ Apr 9, 2016 at 16:57
  • \$\begingroup\$ This code doesn't work, because JavaScript doesn't have String.prototype.contains method. \$\endgroup\$ Apr 10, 2016 at 1:56

1 Answer 1

1
\$\begingroup\$
String.prototype.reverse = function() {
  return this.split("").reverse().join("");
}

Firstly, don't modify objects you don't own. It could cause incompatibility problems if some other code had its own String.prototype.reverse function which had different functionality.

Secondly, this is a bad implementation of reversing a string. For example, calling this function on the following string:

'foo 𝌆 bar'

returns:

'rab �� oof'

You use many meaningless identifiers, for example wc, y, OK, OK2. It's hard to tell what purpose they serve.


while (true)

Instead of using an infinite loop and break-ing it, I recommend you using a do-while loop. In your case it could look like that:

var condition = false
do {
  // the actual loop content

  condition = lastWC === wc
} while (!condition)

See Is while (true) bad programming practice?.


I highly recommend you wrapping your code in an IIFE (see Ben Alman's article for more details). In the current state, all your variables are global, which is a bad practice. See Why are global variables considered bad practice?.


I had to spend quite a while to work out how the replace function works. It seems that you wanted to re-invent the g regex modifier (read MDN RegExp docs). You could get rid of the loop at all if you used it.

I don't see any point in reversing the wc variable two times, except of making the code less readable.

Calling wc.replace() multiple times and assigning the result to wc looks ugly. You could at least chain the .replace() calls like that: wc.replace(...).replace(...).replace(...), or even better, make an array of matches and replacements and loop through them:

var matchesAndReplacements = [
  ["§", ""]
 ,["Ś", ""]
 ,[/\(a(?!\w)/g, "(trela"]
]

for (let i of matchesAndReplacements) {
  let [match, replacement] = i
  wc = wc.replace(match, replacement)
}

And most importantly, this doesn't work. Writing a programming language compiler using regex is a very naive idea, and IMHO even worse than trying to parse HTML with regex. A simple example: what if you had a .l fragment in a string? It would be incorrectly changed to .length.


var source = prompt("Enter Source");

prompt() is a pretty primitive way to make an I/O. Even if you decide so, it shouldn't be somewhere in the middle of the code. Create a function, named compile for example, which takes input as argument and returns output, and then at the end of the code place alert(compile(prompt("Enter Source"))).


var source = prompt("Enter Source");
var sourceCopy = source;

When you declare multiple variables at once, you can use the var keyword only once, and separate the variables declarations with a comma, like that:

var source = prompt("Enter Source")
   ,sourceCopy = source

However, in this case it would be better to use const for source, as you don't intend to modify it.


var y = (a) => {
  for (let char in fullASCII) {
    if (!source.contains(char)) {
      source.replace(a, char);
    }
  }
}

If your arrow function has only one parameter, you don't need to use parentheses, so it could be var y = a=> {}.

Also, this function seems to be no-op, because it doesn't return any value and doesn't assign any value to any variable.


var OK = false;
var OK2 = false;
for(let char in fullASCII){
  if(!source.contains(char)){
    OK2 = OK? true:false;
    OK = true;
  }
}

That looks ugly for me. You could rewrite it like that:

const isOK = fullASCII.map(source.contains.bind(char)).filter(x=>x).length >= 2

One liners FTW!


var sourceSplit = sourceCopy.split("\'");

You don't need to escape apostrophes if you use double quotes to make a string. Besides, I would name the variable sourceSplitted, because now it looks more like a function name.


if (oSS[0] == "§") {

Always use === instead of ==. See Does it matter which equals operator (== vs ===) I use in JavaScript comparisons?.


To conclude: yes, this code is ugly. You have a lot to learn.

\$\endgroup\$
6
  • \$\begingroup\$ I reverse wc because javascript does not have negitive look-behinds, so I use a negitive look-ahead on the reversed string. \$\endgroup\$ Apr 10, 2016 at 19:17
  • \$\begingroup\$ "what if you had a .l fragment in a string? It would be incorrectly changed to .length." I don't run replace() on strings. It's right in the code. I split by quotes and parenthises and run the even indices (0,2,4... etc) because the odd ones are enclosed in quotes. \$\endgroup\$ Apr 10, 2016 at 19:19
  • \$\begingroup\$ Thanks for the help, You have a lot to learn. Yeah, I know, that's why I'm on code review getting advice from nice people like you ^_^. (I hate i when my comment sounds sarcastic but it isnt..) \$\endgroup\$ Apr 10, 2016 at 19:22
  • \$\begingroup\$ @epicTCK What if you had a string enclosed in quotes with a quote character escaped by a backslash: "\""? \$\endgroup\$ Apr 11, 2016 at 8:47
  • \$\begingroup\$ Trace the full ASCII variable. It replaces the escaped quotes with a character not used in the inputted program and remembers that character, <strike>then changes it back</strike> (I forgot to add that last bit in the code. I must have been tired... :P) \$\endgroup\$ Apr 11, 2016 at 14:42

Your Answer

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

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