1
\$\begingroup\$

The function below iterates through an array that contains all the lines of a JavaScript file (a global array, this_file), and returns just those lines (current_function) containing the text of the function with the name that is passed by the argument (funShun). The code assumes that the function to copy ends whenever '}' by itself (not preceded by whitespace) is spotted (this is true of the file I'm working with).

My question is about whether it would be preferable (and how to do it) to substitute a regex in place of iterating through the file line by line.

This works fine, but it seems to me this could be done with a regex applied to the original Ajax string, but that might be beyond my current regex abilities.

All advice greatly welcome!

// Return an array containing the text of the input function.
function load_function_text(funShun) {
  // Iterate through the present JS file's text.
  var line_num = 0; // Track line num so you can grab the previous line.
  var current_function = [];
  var copying_lines = false;
  this_file.forEach(function(line) {
    // Start copying when 'function funShun' is spotted. (Start pushing from previous line.)
    if (line.match("function " + funShun)) {
      current_function.push(this_file[line_num - 1]);
      current_function.push(line);
      copying_lines = true;
      line_num += 1;
    // Stop copying when '}' is spotted.
    } else if (copying_lines && line === '}') {
      // Add the '}' line.
      current_function.push(line);
      // Stop copying when end of function is reached.
      copying_lines = false;
    } else {
      if (copying_lines) current_function.push(line);
      line_num += 1;
    }
  });
  return current_function;
}
\$\endgroup\$
2
\$\begingroup\$

Inefficient Array.forEach

The current implementation will needlessly iterate until the end of the array even after the function was found. It would be better replace the .forEach with a regular counting loop, and break out of it after you found the end of the function.

Avoid flag variables

Using a single loop and the copying_lines flag variable to alternate between processing logic (find start of function, copy content, find end), is not very easy to read. It would be easier to use a nested loop after you found the start. Something like this:

for (let i = 0; i < lines.length; i++) {
  if (lines[i].match(startPattern) !== null) {
    let function_lines = [];

    for (let j = i; j < lines.length; j++) {
      function_lines.push(lines[j]);

      if (lines[j] === '}') return function_lines;
    }

    throw new Error('Could not find end of function before end of content');
  }
}

return [];

Too weak start pattern

This condition is too weak to match the declaration of a function:

if (line.match("function " + funShun)) {

For example, if there are functions named convert_custom and convert, and you call load_function_text('convert'), it will find the first one. I suggest to use a stronger pattern, for example:

startPattern = new RegExp("\\s*function " + funShun + "\\s*\\(");
\$\endgroup\$
1
\$\begingroup\$

janos already covered some parts of your code, however there are a couple other things worth mentioning.

  1. Array.prototype.forEach passes three parameters into the function called for each element of the array. The second parameter is the index number. This index number removes the need to manually keep track of line_num.

    this_file.forEach(function(line, line_num) {
    
  2. While it might not be an issue for the specific file this function was written to handle, if the function happens to be a generator function the current solution would break.

This could be done with a regex as you mentioned, but it won't be very maintainable. Here's a demo that requires that the code is stored as a string.

let code = `
function testA() {
}
function testB () {
  return;
}
function * testC () {

}`

function load_function_text(name) {
  let regex = new RegExp(`function[\\s*]+${name}[\\s\\S]+?\\r?\n\\}`);
  let match = code.match(regex);
  if (match) return match[0].split('\n');
  return []; // Or throw an error
}

['testA', 'testB', 'testC'].forEach(name => {
  console.log(load_function_text(name));
});

A much more robust solution would be to actually parse the JavaScript using a tool like esprima and extract the functions using it. Here is a simple demo:

let source = `function testA() {
}
// Comment
function testB () {
  return;
}

function * testC () {
}

let difficult; function fn() {}; to_get()`;

let fn_cache = {};

esprima.parse(source, {}, (node, meta) => {
  if (node.type == 'FunctionDeclaration') {
    fn_cache[node.id.name] = [meta.start.offset, meta.end.offset];
  }
});

function get_function(name) {
  return source.slice(fn_cache[name][0], fn_cache[name][1]);
}

['testA', 'testB', 'testC', 'fn'].forEach(name => {
  console.log(name, '=>', get_function(name));
});
<script src="https://wzrd.in/standalone/esprima@latest"></script>

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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