2
\$\begingroup\$

I've written an algo to compress a string

eg. aabcbbba to a2bcb3a

I'm pretty new to functional programming, I feel it's easier to debug and learn from functional code if you're good at it already, but total opposite if you're not.

I'm wondering how I can make this code cleaner and more functional.

I feel like there's gotta be a better way to do compressCharacters without the need of an array or a result variable (perhaps substituting forEach with something else) as well as reducing the lines of code in groupCharacters

const signature = `aabcbbba`;

const compressString = signature => {
  return compressCharacters(groupCharacters(signature));
}

const groupCharacters = signature => {
  let newSignature = "", arr = [];
  
  // convert signature to an array
  let result = [...signature].reduce((accumulator, element, index) => {
    
    // check if last letter in accumulator matches the current element
    if (accumulator[accumulator.length -1] !== element) {
      // add the accumulator string into an array
      arr.push(accumulator);
      // clear the newSignature string and set it to current element
      newSignature = element;
    } else {
      // if it doesn't match, add it to the accumulator
      newSignature = accumulator += element;
    }
    
    // check if it's the last item - add to array
    if (index === signature.length - 1) arr.push(element);
    
    // return newSignature to accumulator
    return newSignature;
  })

  return arr;
}

const compressCharacters = arr => {
  let newArray = [];

  let result = arr.forEach(e => e.length > 1 ? newArray.push(`${e[0]}${e.length}`) : newArray.push(e))

  return newArray.join("");
}

compressString(signature);

\$\endgroup\$
3
\$\begingroup\$

Style notes

  • Not putting {, } around single line statement blocks is a bad habit. Always delimit statement code blocks.
  • In compressCharacters newArray should be a constant. In groupCharacters arr should be a constant.
  • Consistent naming is important. Your naming is all over the place. You call a string, characters (in compressCharacters), string (in compressString), signature, and e in the forEach. A character you call element. You abbreviate an array to arr in one function and call it newArray in another. Most of the names are describing the type and not the abstracted data that they hold.
  • Don't add useless or redundant code. In compressCharacters you create a variable result that you do nothing with. Not to mention that forEach does not have a return defined. Also result in groupCharacters is never used.
  • Don't declare variables outside the scope that they are to be used. newSignature is only used inside reduce but declared outside the callback.

Functional programing means that functions should not have side effect (change the state of anything outside the function's scope.) The reduce callback uses the array arr which breaks the no side effects rule. And the forEach pushes to arr which is also outside the forEach callbacks scope (use map or reduce in that case).

Applying the above you would get something like the following code.

const groupRuns = str => [...str].reduce((groups, char) => {
        const last = groups.length - 1;
        if (last < 0 || groups[last][0] !== char) { groups.push(char) }
        else { groups[last] += char }
        return groups;
    }, []);

const concatGroups = groups => groups.reduce((str, g) => 
        str + (g.length > 2 ? g[0] + g.length : g)
    , "");

const compressString = str => concatGroups(groupRuns(str));

const signature = `aabcbbbaaaaaabcccccdddddddbbba`;
compressString(signature);

Note that rather than add numbers to groups of size 2 the group must be 3 or larger.

\$\endgroup\$
  • \$\begingroup\$ g.length > 2 is wrong: it fails to transform aa to a2 as specified. \$\endgroup\$ – 200_success Nov 13 '18 at 20:48
  • \$\begingroup\$ @200_success As I pointed out. \$\endgroup\$ – Blindman67 Nov 13 '18 at 20:56
  • \$\begingroup\$ shouldn't concatGroups also be delimited with brackets and returns? \$\endgroup\$ – totalnoob Nov 13 '18 at 22:04
  • \$\begingroup\$ @totalnoob It is not required, in fact groupRuns can do without the return. However I should have moved the returning object to the same line. I will update it now. \$\endgroup\$ – Blindman67 Nov 14 '18 at 0:55
  • 1
    \$\begingroup\$ @totalnoob Very simple... extending from JollyJoker's.comment the function would be const compressString = str => str.replace(/(.)\1+/g, str => str[0] + str.length); and used like so compressString("aabcbbba"); returns "a2bcb3a" \$\endgroup\$ – Blindman67 Nov 14 '18 at 17:46

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.