1
\$\begingroup\$

I'm trying to write a constructor function that generates instances of a user. One of the properties of the user class is an array of the user's hobbies. I'd like my constructor to have a method that generates a string representation of the array of hobbies provided to the constructor function. The goal is to create a grammatically correct sentence containing the hobbies of the Person instance, so if interests = ['hiking', 'biking', 'skiing'] the Person.bio() method will alert something like: This person's interests are: hiking, biking and skiing. I am trying to account for an array of unknown length being passed to the constructor.

I haven't used .reduce() much but from the little bit of testing I've done, this seems to do what I want it to do. I'm just looking for any critiques on how to make things more readable or performant!

Below is a stripped down version of the constructor function (missing things like name, age, etc. for clarity).

function Person(interests = []) {
  this.interests = interests;
  this.hobbiesSentence = interests.reduce((hobbyString, hobby, index, interests) => {
    switch(index) {
      case (interests.length - 1):
        return hobbyString += `${hobby}.`
      case (interests.length - 2):
        return hobbyString += `${hobby} and `
      default:
        return hobbyString += `${hobby}, `
    }
  }, '');

  this.bio = function () {
    alert(`This person's interests are: ${this.hobbiesSentence}`)
  };
}

As I'm writing this, I could see a case for making the function exist but not necessarily creating/storing the sentence unless the bio() method is called. Any other ideas? Thanks in advance.

\$\endgroup\$
  • 2
    \$\begingroup\$ The word "implementation" is misleading; it made me think this was a reinventing-the-wheel question. Perhaps "usage" would make more sense here? \$\endgroup\$ – Alex F Aug 8 at 0:14
  • 1
    \$\begingroup\$ This question is pretty borderline. In the future, please don't simplify your code for the sake of posting on Code Review. See How to Ask. \$\endgroup\$ – 200_success Aug 8 at 2:26
  • 1
    \$\begingroup\$ @200_success reviewed for future reference, thanks. \$\endgroup\$ – Anthony Aug 8 at 20:45
4
\$\begingroup\$

Performance

Strings are immutable, so using accumulator += stuffToAppend in a loop can traditionally impact performance. The problem is that we're creating a new string every iteration, leading to quadratic time complexity for an operation that should be linear. It turns out that modern browsers optimize this heavily using an internal array to represent string parts and make it quite fast over using an explicit array, so this post is focused on style rather than performance.

Design

On first thought, reduce seems like the right function from a semantic standpoint since we want to boil the array of interests down to one string. However, since avoiding string concatenation requires an intermediate array in reduce, we might as well just skip the intermediate array and use map and join. It's pretty common that reduce can be replaced with map or filter, which are more specific and succinct.

Switch statements are also generally not used much in JS (but often used in C...). You can replace many switch statements in JS with an object (particularly if you're choosing between a number of similar functions), or at least an if statement. Either way, the nature of the commas and "and" in this example makes it a bit awkward, so there doesn't seem to be any clear-cut win.

Additionally, this routine of "prettifying" a list is generic and can be moved to a separate function to keep Person clean.

As an aside, instead of switching between "interests", "hobbies" and "bio", it seems best to pick one term and stick with it throughout.

Here's my attempt. This might seem a bit abstract, but it's typical in JS to avoid conditional/switch stuff as much as it is to avoid loops (which is the idea with reduce). If you prefer a more traditional approach, replace the joins array and indexing with an if statement and I'd still endorse it.

const prettyList = (a, sep=", ", endSep=[" and ", "."]) => 
  a.map((e, i) => e + (endSep[endSep.length-a.length+i] || sep)).join("")
;

const Person = function (interests=[]) {
  this.interests = interests;
  this.interestsSentence = prettyList(interests);

  this.interestsStr = () => 
    "This person's interests are: " + this.interestsSentence
  ;
};

const interests = ["foo", "bar", "baz", "quux"];

for (let i = 1; i <= 4; i++) {
  console.log(new Person(interests.slice(0, i)).interestsStr());
}

Now the function is reusable and we can change its behavior without much effort at all:

const prettyList = (a, sep=", ", endSep=[" and ", "."]) => 
  a.map((e, i) => e + (endSep[endSep.length-a.length+i] || sep)).join("")
;

const activities = ["biking", "running", "walking", "skipping", "driving"];
console.log("I love", prettyList(activities, "; ", [", sometimes ", " but not ", "!"]));
console.log("I love", prettyList(activities, " and ", [" while ", " :-o"]));

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for taking the time to give some real feedback. As you might expect, I'm pretty green at coding and I posted here looking for constructive ways to write more "real world" JS. OOP is new to me as well, so I wrote this with the idea that everything necessary to modify the "person" instances would be contained on the constructor object. I'll dig through your reply to make sure I understand the nuances. \$\endgroup\$ – Anthony Aug 8 at 20:38
  • \$\begingroup\$ To me it seems that the string concatenation in your map is the same as it would be in a reduce (or just +). Also, what do you mean by "intermediate array"? \$\endgroup\$ – morbusg Aug 12 at 1:11
  • \$\begingroup\$ The difference is taking an accumulator string and resultStr += elem versus building an array of elem + someExtraStuff, then joining it for the result. Traditionally, string concatenation is much slower because a totally new string is built on every iteration. Looking into it, it turns out that browsers have optimized it considerably, so it's probably not a major issue as it might be elsewhere. By "intermediate array", I mean that if we want to avoid string concats, we pass [] as the initial accumulator for .reduce and append each element--but wait, that's exactly what map does! \$\endgroup\$ – ggorlen Aug 12 at 1:20
  • 1
    \$\begingroup\$ Somehow, in my mind I thought: "What is join if not string concatenation", but I don't actually know what happens "under the hood". Also I was thinking giving an empty string as the initial value for the reducer (no need to assign to the accumulator there BTW, since the return value will be passed to the next invocation). But anyway, I agree that the reduce is a bit unwieldy here. I played around with this a little and actually ended up using a switch myself :D \$\endgroup\$ – morbusg Aug 12 at 2:00
  • \$\begingroup\$ It turns out that modern browsers optimize this heavily using an internal array it seems we no longer need expert developers in this world. Bad code gets optimized anyway :p \$\endgroup\$ – dfhwze Aug 12 at 3:51

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.