3
\$\begingroup\$

I'd like to know what's the best practice as functional programming in JavaScript to work with arrays and passing them as an argument to a recursive function that may run itself within its branch.

In my code I'm trying to recursively find all typescript files that are imported/exported from an entry point by calling preprocessTSFiles. To do this, I read the entry and crawl to all other files. This leads to a recursive call of getChildren and I don't want to call handleFile twice on the same file.

I tried this approach but I wanted an insight of expert in functional programming (please note that I'm not very familiar with functional programming and bear with me):

function copyFile(file: VinylFile, destFilePath: string): void {
  const destDir = path.dirname(destFilePath);
  ensureMakeDir(destDir);
  writeFile(destFilePath, file.contents.toString());
}

function getVinyl(filePath: string): VinylFile {
  const contents = new Buffer(readFile(filePath));
  return new vinyl({
    base: path.dirname(filePath),
    path: filePath,
    contents,
  });
}

function getChildren(file: VinylFile): VinylFile[] {
  const statementRE = /(import|export)\s*((\{[^}]*\}|\*)(\s*as\s+\w+)?(\s+from)?\s*)?([`'"])(.*)\6/ig;;
  const result: VinylFile[] = [];

  let match: RegExpMatchArray | null;
  while ((match = statementRE.exec(file.contents.toString())) && !isNil(match)) {
    const inFile = match[7];

    if (!isEmpty(inFile) && inFile[0] === '.') {
      let resolvedPath = path.resolve(file.base, inFile);
      if (/\.ts$/.test(resolvedPath) === false) {
        resolvedPath += '.ts';
      }
      if (isFile(resolvedPath)) {

        result.push(getVinyl(resolvedPath));
      } else if (isDirectory(resolvedPath)) {

        debugger;
        const indexBarrel = path.resolve(resolvedPath, 'index.ts');
        if (isFile(indexBarrel)) {
          result.push(getVinyl(indexBarrel));
        }
      }
    }
  }
  return result;
}

async function handleFile(file: VinylFile, destDir: string, baseDir: string, plugins: HandlerPlugin[], processedList: string[]): Promise<void> {

  for (const child of getChildren(file)) {
    if (processedList.indexOf(child.path) < 0) {
      processedList.push(child.path);

      await handleFile(child, destDir, baseDir, plugins, processedList);
    }
  }

  if (isArray(plugins)) {
    for (const plugin of plugins) {
      const result = await plugin(file);

      if (!isEmpty(result) && result !== file.contents.toString()) {
        file.contents = new Buffer(result);
      }
    }
  }

  const destFilePath = file.path.replace(baseDir, destDir);
  copyFile(file, destFilePath);
}


export function preprocessTSFiles(entryFilePath: string, destDir: string, baseDir?: string): Promise<void> {
  const fileList: string[] = [];
  const plugins = [InlineTemplate, InlineStyles];

  if (isNil(baseDir)) {
    baseDir = path.dirname(entryFilePath);
  }

  return handleFile(getVinyl(entryFilePath), destDir, baseDir, plugins, fileList);
}
\$\endgroup\$
  • \$\begingroup\$ Are you using flow for types? In this case please add the head line, so to make the code mode clear. And could you post VinylFile definition? \$\endgroup\$ – Mario Santini Aug 22 '17 at 4:45
  • \$\begingroup\$ I'm using typescript and tslint. \$\endgroup\$ – PRAISER Aug 22 '17 at 10:06
  • 1
    \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. cc @MarioSantini \$\endgroup\$ – Vogel612 Aug 22 '17 at 10:24
  • \$\begingroup\$ @Vogel612 thanks for the heads-up, I'm gonna read the rules now. \$\endgroup\$ – PRAISER Aug 22 '17 at 10:30
3
\$\begingroup\$

Update

As the OP is been updated and changed I want to change my answer, I'll live the original at the end.

I see you use Object Oriented other than functional, so I'll comment some I saw that is not ok for me.

Encapsulation broken

Looking at your code I see the broken of encapsulation. This means that from a piece of code you access or worst you try to change some internal data of an object.

You should not do that. Rather you should improve the object to provide a propper interface.

Here is an example from your code in handleFile():

...
const destFilePath = file.path.replace(baseDir, destDir);
copyFile(file, destFilePath);
...

The issue here is you have to know to that baseDir should match with the file path attribute.

A better way would be just:

copyFile(file, destDir);

in this case you conain inside the copyFile() function the details about using the path.replace... staff. So you avoid to copy a file in another part of your source code and forgot this part.

Another reason to put the logic inside the copyFile() function is that you could actually remove that line:

function copyFile(file: VinylFile, destFilePath: string): void {
    const destDir = path.dirname(destFilePath);
    ensureMakeDir(destDir);
    writeFile(destFilePath, file.contents.toString());
}

I think you just want to place the file name at the end of the new path. The first line of the function here, you just do a rollback of this part.

function copyFile(file: VinylFile, destDir: string): void {
    ensureMakeDir(destDir);
    const destFileName = path.join(destDir, path.basename(file.path));
    writeFile(destFileName, file.contents.toString());
}

This is a little bit better because you contain the VinyFile details in a single function, and you could change it without worring that something will broke in your code.

But is not the best! A better solution will be if the file know how to copy to another path, in such a way you should not deal with the object internal details.

You should be able to do:

file.copyTo(destDir);

If you try to write such a method you will notice that the code needed is simplest and much clear.

And you could maintain all the object boundary safe from the code base.

Another small thing here. I saw you use the path package before, so please avoid some trick like:

file.path.replace(baseDir, destDir);

It is difficult to understand and easy to broke.

Also the getVinyl() could be removed, the VinylFile should know how to load his content properly.

Here on getChildren(): while ((match = statementRE.exec(file.contents.toString())) && !isNil(match))

Correction

I checked the Javascript RegEx documentation of exec and in the examples they show a case where you use exec() with the option g. In this case you could have a loop as the regex object save the lastIndex and consider just a substring.

Anyway, if you remove g option exec() will behave differently, and that is dangerous.

For that reason I found out that the following sentence is not accurate.

Not sure why you use a loop as exec should return the matching. As the file content is not changing you will loop forever.

And this part I'm sure is not working:

    debugger;
    const indexBarrel = path.resolve(resolvedPath, 'index.ts');

There is still the debugger... :) I think you run this code on server side so that line have no effects, but please remove it, just in case.

The getChildren() function should be moved as a method of VinylFile. Here I use only es6 syntax as I don't know Typescript, but I think it will be easy to move on the proper syntax.

// should be a VinylFile attribute or a private const
const statementRE = /(import|export)\s*((\{[^}]*\}|\*)(\s*as\s+\w+)?(\s+from)?\s*)?([`'"])(.*)\6/ig;

hasChildren() {

   if (subFilesMatch != null) {

   }
   return false;
}

getChildren(): VinylFile[] {
    const children: VinylFile[] = [];

    let subFilesMatch = statementRE.exec(this.contents.toString()));
    if (!isNil(subFilesMatch)) {
        const filePath = subFilesMatch[FILE_PATH_MATCH_INDEX];

        // Again don't use triks rather use the packages features!
        if (path.extname(filePath) !== '.ts') {
            filePath += '.ts';
        }
        const childFileName = path.join(this.base, filePath);
        if (isFile(childFileName)) {
            // Here I just rearrange your code, but you have an issue here!
            // I don't know how many children you have on each file instance
            // but *getChildren()* is synchronous function, so the file 
            // content load is blocking!
            const contents = loadContents(childFileName);
            const opts = {base: this.base, path: childFileName, contents}
            children.push(new VinylFile(opts));
        } else if(isDirectory(childFileName)) {
            ...
        }
    }

    return children;
 }

As prompted in the comment above, your method here, even the original function, is blocking code, as it load the file content of the children sinchronously.

You should convert this too in an async function. I don't know the syntax in Typescript.

Last but not last handleFile() It is an async function and should return a promise and it doesn't.

Maybe Typescript is honoring the function signature and returning a promise in any case, and, I guess, in a working like mode, so you think the code is fine.

Please check this part too.

As I said before, you should create an array of promises and return it from getChildren(), and then return a Promise.all() from handleFile().

I'll do this in javascript.

Original answer

I'm not a big expert of functional programming, but your example did not fit.

I guess so.

The first point is that you create a list of random numbers. This end up in a different list each time.

In functional programming, a function should take input and produce output, and given the input it should return always the same output.

So, you just set up a trivial example, but in my above definition you just could notice other issues.

The parameter name resultList, is not the "resultList", is just the start list that is going to be fit of 10 random numbers.

function SampleRecursiveFunction(resultList) {
  // copy our list
  const result = [...resultList];

Instead of resultList you should have a different name like: inputList.

Another bad name is result.

You should use some much helpfull like: randomNumberList

  function SampleRecursiveFunction(inputList, maxLength = 10) {
      if (inputList.length >= maxLength)
          return [...inputList];

      let randomNumber = Math.random()*100;
      while(inputList.length > 0 && 
            inputList.indexOf(randomNumber) !== -1)
          randomNumber = Math.random()*100;

      return SampleRecursiveFunction([...inputList, randomNumber], maxLength);
  }

This is a small evolution of your function with recursion.

The function accept a new parameter that set the returned list max length.

An even better version could be like:

function GetRandomNumber(seed) {
    return function () {
        return Math.random() * seed;
    }
}

This function return a function that provide the proper next value.

In real case you could have a generation function instead, depending on what you're doing.

function SampleRecursiveFunction(inputList, getNewNumber, maxLength = 10) {
      if (inputList.length >= maxLength)
          return [...inputList];

      let randomNumber = getNewNumber();
      while(inputList.length > 0 && 
            inputList.indexOf(randomNumber) !== -1)
          randomNumber = getNewNumber();

      return SampleRecursiveFunction([...inputList, randomNumber], maxLength);
  }

And you could compose the function like this:

  console.log(SampleRecursiveFunction([], GetRandomNumber(100)));

The good of this is you could split your logic into small tasks and put them in functions. At the end you compose those functions and building your whole logic.

As we deal with random numbers, even my example is not pure functional, I just try to give you a review of your code example.

I think you should find a more detailed example on what you're tring to do to have a much usefull help.

\$\endgroup\$
  • \$\begingroup\$ You had some good notes, but I updated my question, please review. Thanks in advance \$\endgroup\$ – PRAISER Aug 21 '17 at 22:23
  • \$\begingroup\$ @PRAISER I saw your update and change my answer accordingly. Hope all is readable. Anyway: just ask. \$\endgroup\$ – Mario Santini Aug 22 '17 at 16:25
  • \$\begingroup\$ Okay there's a few things to mention: 1. The destDir is not exactly the full path of new file that's why I was replacing the beginning part so i keep the rest of path in new destination path. 2. The loop for RegEx is needed and it's for if there is more than one statement in the file we catch them all in that loop. \$\endgroup\$ – PRAISER Aug 22 '17 at 23:25
  • \$\begingroup\$ 3. If I move my logic of getting children in vinyl class then it not functional programming anymore, to me that's imperative, correct me if I'm wrong. 4. Where the logic of not processing if one file is already processed go? And regarding missing return of a promise, an async function doesn't need to return a promise, the type in typescript is needed though. Also i wanted to fix some part of my code but rules here don't allow me to, It's awful that I cannot update my question! \$\endgroup\$ – PRAISER Aug 22 '17 at 23:25
  • \$\begingroup\$ @PRAISER about point 1 the issue is that the code you use it's difficult to understand and to change. I proposed some different approach that help the reading and more easy to change. About point 2 I correct the answer. \$\endgroup\$ – Mario Santini Aug 23 '17 at 6:48

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.