DEV Community

Cover image for Review My Code Please
Jaden Concord
Jaden Concord

Posted on

Review My Code Please

I am working on a little project using JavaScript (it's a work in progress and part of a bigger project). I would appreciate it if anyone could review any of the following code and criticize and tell me what I may be doing wrong. I am trying to code in a way where the code explains what is going on so if you don't understand, what is confusing?

let TemplateBlocks = {
  eval: a => '`;result+='+a.content+';result+=`',
  repeat: a => {
    let i = a.args[1] || 'i';
    return '`;for(var '+i+'=1; '+i+'<'+(Number(a.args[0])+1)+'; '+i+'++){result+=`'+a.content+'`;}result += `';
  },
  var: a => '`;var ['+a.perams.keys()+'] = ['+a.perams.values().reduce((b,c) => b+'"'+c+'",','')+'];result += `',
  if: a => '`;if('+a.perams.condition+'){result+=`'+a.content+'`;}result += `',

  defaultBlock: a => {
    console.warn('Template block "'+a.name+'" is not defined');
    return '';
  },
}

////////////////////////////////////////////////////////////////////////////////

function Templater(template){
  let result = FormatTemplateVariables(template);
  result = ReplaceTemplateBlocks(result);
  return EvalTemplate('let result = `'+result+'`; return result;');
}

function FormatTemplateVariables(template){
  // {js} -> {eval}js{/eval}
  let regex = /{(([^{/ ]*?)( [^{]*?)?)}(?!.*{\/\2}.*)/gs;
  return template.replace(regex, '{eval}$1{/eval}');
}

function ReplaceTemplateBlocks(template){
  // {block arguments}content{/block}
  let regex = /{(\w+)( [^}]+)?}((?!.*{(\w+)( .*?)?}.*{\/\4}.*).*){\/\1}/s;
  return RegexReplace(template, regex, ReplaceBlock);
}

function EvalTemplate(text){
  try {
    return Function('', text)()
  }
  catch (error){
    console.error("Error Evaluating template: "+error.message+'\n\n'+
      text.split('\n')[error.lineNumber-1]+'\n'+" ".repeat(error.columnNumber-1)+'^\n');
    return '';
  }
}

function RegexReplace(text, regex, getReplacement){
  let safe = 10000;
  let result = text;
  while(result.match(regex) && --safe > 0){
    let match = result.match(regex);
    result = result.replace(match[0], getReplacement(match))
  }
  safe <= 0 && console.error('RegexReplace: loop has no end');
  return result;
}

function ReplaceBlock(match){
  let arg = (match[2]||' ').substring(1);
  let [content, name, perams, args] = [match[3], match[1], GetPerams(arg), arg.split(' ')];
  let func = TemplateBlocks[name] || TemplateBlocks.defaultBlock;
  let exportData = { arg,content,name,perams,args,func };
  return func(exportData);
}

function GetPerams(text){
  if(!text)return {};
  let regex = /(\w+)(="(.*?)")?/ // name="value" or name
  let result = {}
  RegexReplace(text, regex, match => {
    result[match[1]] = match[3] === undefined ? true : match[3];
    return '';
  });
  return result;
}

// For testing
$$('template').forEach(el => {
  el.outerHTML = '<div>'+Templater(el.innerHTML)+'</div>';
})

Enter fullscreen mode Exit fullscreen mode

Example

      <template>
        <p>Welcome, this is a templating test. Now we will set some variables...</p>
        {var name="joe" age="24" born="1986"}{/var}
        <b>Hello {name}, you were born in 1986</b>
        <p>We know that you {age ? 'are a child beacuse your under 18' : 'are an adult because you are older than 18'}</p>
        <p>Now we will loop through something</p>
        {repeat 10 a}
          <br>
          <hr>
            <h1>Hello, this is item {a}</h1>
            <p>What we know is that you are on item number {a} and that {a} squared is {a*a}
            Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
            We made a special color button for you,</p>
            <button style="background: {'#'+(a*2)+a+a}; color: white" onclick="alert('you clicked button {a}')">Button {a}</button>
          <br>
        {/repeat}
        <p>lots of work to be done...</p>
        {repeat 10}
        Hello {i}
        {/repeat}
        {test}hello{/test}
      </template>
Enter fullscreen mode Exit fullscreen mode

Result

<div>
        <p>Welcome, this is a templating test. Now we will set some variables...</p>

        <b>Hello joe, you were born in 1986</b>
        <p>We know that you are a child beacuse your under 18</p>
        <p>Now we will loop through something</p>
          ...
          <hr>
            <h1>Hello, this is item 10</h1>
            <p>What we know is that you are on item number 10 and that 10 squared is 100
            Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
            We made a special color button for you,</p>
            <button style="background: #201010; color: white" onclick="alert('you clicked button 10')">Button 10</button>
          <br>

        <p>lots of work to be done...</p>

        Hello 1

        ...

        Hello 10

        undefined
      </div>
Enter fullscreen mode Exit fullscreen mode

Top comments (9)

Collapse
 
jamesthomson profile image
James Thomson

A few things that stand out

  • Don't use reserved words
  • Using eval is extremely dangerous and highly discouraged
  • It's highly discouraged to reassign/mutate a function parameter's value. Instead of doing this, make a copy and mutate that.
  • Use descriptive parameter names so we don't have to read the function to understand what value it accepts. e.g. Your ReplaceTemplate function has one parameter. How are we to know that this parameter should be an array? Also, how are we to know how this array should be constructed?
  • As other have said, use ES6 variable declarations. i.e. const and let. var should be considered deprecated

Hopefully this is the kind of feedback you were looking for. Best of luck with your project!

Collapse
 
jadenconcord profile image
Jaden Concord

Will using window.Function instead of eval be better? Also, I have an experimental regular expression that blocks running functions or setting variables in strings. Would that work along with switching to using Function? Thanks so much for your help!

Collapse
 
jamesthomson profile image
James Thomson

Yes, using Function is the recommended alternative.

I have an experimental regular expression that blocks running functions or setting variables in strings.

I don't see why it wouldn't work, but I couldn't say for sure.

Collapse
 
jadenconcord profile image
Jaden Concord

Thank you so much!

Collapse
 
robertseidler profile image
RobertSeidler • Edited

Hello,

So, I like the way you organize your functions. Both the order they appear in and the average size are reasonable. Also the names of your functions are clear and reflect a single task. All of those things are much better and more disciplined then my day to day js code.

You did not mention the overall goal of your code, so I don't really understand the idea of the "TemplateObject". Maybe an example of what goes into "Templater" and what comes back out would make it clearer. Or instead of testing your code by applying it to some html document, you could just run that test on a string, that is included (atleast the input would then be clear; output can be estimated by following the code).

You use camel-case with capitalized function names. Usally js uses non-capitalized camel-case for functions (considering the clarity of your function names not too impactfull).

Then their are a lot functions with "a" as parameter. And "a" is used for different types of things. In "ReplaceTemplate(a)", "a" are matches; in "RegexReplace(a, ...)" it is a string. In my opinion, the function definition is the most important place, to find good names for. So my suggestion would be: "ReplaceTemplate(a)" -> "ReplaceTemplate(matches)" and "RegexReplace(a, ...)" -> "RegexReplace(sourceStr, ...)" or even "RegexReplace(text, ...)" like you did in other functions earlier.

In "TemplateObjects":
I'm a little confused about the the functions defined as his members. For example in the first member "eval: a => ... + inner + ...". You declare, that eval accepts a parameter "a", but don't use it inside the function. Instead you have "inner", for which I have no idea, where it is coming from. I would expect from some kind of eval related function to look like: "eval: serializedCode => ... + serializedCode + ..." or in case the "inner" is external and independent of paramter: "eval: () => ... + inner + ...". Some of the other members are similar.

I hope, that is what you expected, when asking for a review. I'm not doing those all that often, because I have to work alone most of the time. It was fun, tho.

All in all, I think your code is understandable and clean for the most part. Improvements are possible, personally I often am too lazy to keep my code that nice and sometimes I hate myself for that. xD

Collapse
 
jadenconcord profile image
Jaden Concord

Yes, thank you so much that was so helpful! Thank you!

Collapse
 
yas46 profile image
Yasser Beyer

Learn the basics. Don't give up.

youtu.be/PkZNo7MFNFg

Some comments may only be visible to logged-in visitors. Sign in to view all comments.