DEV Community

Cover image for Review My Code Please

Review My Code Please

Jaden Concord on January 07, 2021

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 rev...
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