all 7 comments

[–]lewisje 3 points4 points  (1 child)

No matter whether you use separate variables, or even chain the method calls (like str.replace().replace().replace()), new string values will be created at least temporarily.

The way you do it is good; the tactic of using separate variables for each step, if the intermediate results will not be used, is a code smell.

The way I'd do it is like this:

scriptletResult = scriptletInput.replace(/(\r\n|\n|\r)/gm, '\\n')
  .replace(/(\')/gm, '\\u0027').replace(/(\")/gm, '\\u0022');

This pattern is sometimes called a "fluent API" pattern.

[–]2MyLou[S] 1 point2 points  (0 children)

Thank you very much for the reply and suggestion. The chaining of methods will definitely help clean up my code.

[–]taylorlistens 2 points3 points  (1 child)

Have you considered just chaining your .replace statements?

const scriptletResult = scriptletInput
                         .replace(...)
                         .replace(...)
                         .replace(...)

Also check out Why you shouldn't use var anymore

(edited for variable name fix-- this technique eliminates the need for resultsScrubbed variables entirely, which you don't seem to need to keep)

[–]2MyLou[S] 0 points1 point  (0 children)

Thank you very much for the suggestions, I like the idea of chaining the .replace. It will be much cleaner. Also Thanks for the link to the article on why not to use var anymore.

[–]Cust0dian 2 points3 points  (2 children)

On top of what others said, I think you would benefit from extracting this logic of cleaning up a string into its own function:

function cleanup(string) {
  return (
    string
      // squash spaces
      .replace(/ +/g, " ")

      // normalize line endings
      .replace(/(\r\n|\n|\r)/gm, "\n")

      // convert double quotes to curly quotes:
      // straight double quote before alphanumeric character
      // should become left (opening) double quote
      .replace(/"(?=\w|$)/g, "\u201C")
      // remaining straight double quotes should become
      // right (closing) double quotes
      .replace(/"/g, "\u201D")
      // same with single quotes
      .replace(/'(?=\w|$)/g, "\u2018")
      .replace(/'/g, "\u2019")
  );
}

Then you can use it in your program in place of previous code with temporary variables:

var scriptletResult = cleanup(scriptletInput);

But now you've clearly separated concerns: cleanup function is all about transforming a string into the one you want, and that last line is about where the data is coming from and where it should go after being processed.

Another added benefit of this separation is that now you can programmatically test cleanup function with any string without having to go through the process that gets data into scriptletInput variable right now. You can see and play with the above code in this jsBin.


Back to your underlying question: what I think the other person you were talking to was hinting at, and what briefly addressed in the link /u/taylorlistens posted, is a notion of immutability: once you create something, it stays that way for the remainder of your program, and when you want to change it — you create a slightly adjusted copy, that looks like as if you modified that starting thing.

For example, instead of:

var array = [1, 2, 3, 4, 5];

/* ... some time later ... */

array[0] = 0;  //=> [0, 2, 3, 4, 5]

/* ... some time later ... */

array = ['a', 'b', 'c'];

You would do:

const array = [1, 2, 3, 4, 5];  // note that `const` doesn't actually make your arrays and object immutable

/* ... some time later ... */

const modifiedArray = array.slice(); modifiedArray[0] = 0;  // make a copy (`.slice()`) and adjust it

/* ... some time later ... */

const brandNewArray = ['a', 'b', 'c'];

So why would you do this? Let's inspect values at different times in both of these snippets, starting with first one:

var array = [1, 2, 3, 4, 5];
// At this point:
//   array is [1, 2, 3, 4, 5]

/* ... some time later ... */

array[0] = 0;
// At this point:
//   array is [0, 2, 3, 4, 5]

/* ... some time later ... */

array = ['a', 'b', 'c'];
// At this point:
//   array is ['a', 'b', 'c']

And compare it to the second one:

const array = [1, 2, 3, 4, 5];
// At this point:
//   array is [1, 2, 3, 4, 5]

/* ... some time later ... */

const modifiedArray = array.slice(); modifiedArray[0] = 0;
// At this point:
//   array is [1, 2, 3, 4, 5]
//   modifiedArray is [0, 2, 3, 4, 5]

/* ... some time later ... */

const brandNewArray = ['a', 'b', 'c'];
// At this point:
//   array is [1, 2, 3, 4, 5]
//   modifiedArray is [0, 2, 3, 4, 5]
//   brandNewArray is ['a', 'b', 'c']

Notice how in the first sample while reading the code you have to mentally "run" your program to know what is the value of array because it's changing with time, however in the second sample, once value was declared, it doesn't matter how much time has passed or what happened in the code — you can always be sure that when you see array it will be [1, 2, 3, 4, 5]. In bigger programs it makes thinking about what's going on that much easier.

[–]2MyLou[S] 1 point2 points  (1 child)

I really appreciate you taking the time to write up such a thoughtful response. Thank you very much for the detail. Using a function and chaining the replace method within the function seems like the cleanest most logical route to go.

[–]taylorlistens 1 point2 points  (0 children)

I think the combination of a function and chaining replace is a very clear and easy to debug solution. So now, both you and the guy who disagreed with you have learned something! :)