2
\$\begingroup\$

this is actually my first question so please don't go hard with me. I am in the process of learning my Javascript skills and I am wondering if there are some valuable suggestions to better write the below code to be more optimized/readable etc?

I didn't put any comments within the code itself but what it basically does is return an Array of sports events (I believe that it is quite self-descriptive).

So, would you leave it as is, break it into smaller methods, or change something else, etc.

The question is a little bit "subjective" but in general I would like to know if I am on a good path with this.

Thanks a lot in advance.

    const events = response.events.map((event) => {

        const dateTime = getDateTime(event.start);

        return {

            id: event.id,

            date: dateTime.slice(0, 10),

            time: dateTime.slice(11, 19),

            name: event.name.trim(),

            participants: event["event-participants"].map((participant, index) => {

                return {
                    id: participant.id,
                    name: participant["participant-name"].trim(),
                    sortOrder: index + 1
                };
            }),

            metaTags: event["meta-tags"].map((metaTag) => {

                return {
                    id: metaTag.id,
                    type: metaTag.type.trim(),
                    name: metaTag.name.trim()
                }
            }),

            markets: event.markets.map((market) => {

                return {

                    id: market.id,
                    name: market.name.trim(),

                    selections: market.runners.map((selection) => {

                        return {

                            id: selection.id,
                            name: selection.name.trim()
                        };
                    })
                };
            })
        };
    });
\$\endgroup\$
4
  • \$\begingroup\$ When I run the code it does not return an Array of sports events. 😶 \$\endgroup\$ Jun 2, 2022 at 7:03
  • \$\begingroup\$ Thanks KIKO S. The code is working. 'response' variable is populated with API Call to some endpoint but the Array content itself is not that important. What bothers me the most is whether this code is visually simple/readable enough from the point of some experienced developer. \$\endgroup\$ Jun 2, 2022 at 7:11
  • \$\begingroup\$ Yes, it is readable. It turns an existing array into a very similar, yet slightly different, array. Perhaps the original array contains much more information, and this is your way to extract only the information you need? That is what it seems to do. Whether you should move the value of, for instance markets, to a separate function expression is a matter of opinion. If the only purpose is extraction, you might forego most of the code and just say: markets: event.markets. \$\endgroup\$ Jun 2, 2022 at 7:36
  • \$\begingroup\$ @KIKO Software - exactly! The main purpose of the function is to "filter" an existing array (which contains lot of non-relevant data) and to adjust the extraction process to match destination Model/Database design. Have a nice day and thank you once again. \$\endgroup\$ Jun 2, 2022 at 8:05

2 Answers 2

5
\$\begingroup\$

There is nothing wrong with the structure of the code, but we have to 'read' the code to figure out what it is doing. Put the mapping function (the inline callback) into a discreet, named function that tells anyone reading the code what it does. I'm guessing what the data types might be here, but hopefully you'll get the gist:

function rawEventToEventModel(event) {

  const dateTime = getDateTime(event.start);

  return {

    id: event.id,

    date: dateTime.slice(0, 10),

    time: dateTime.slice(11, 19),

    name: event.name.trim(),

    participants: event["event-participants"].map((participant, index) => ({
      id: participant.id,
      name: participant["participant-name"].trim(),
      sortOrder: index + 1
    })),

    metaTags: event["meta-tags"].map((metaTag) => ({
      id: metaTag.id,
      type: metaTag.type.trim(),
      name: metaTag.name.trim()
    })),

    markets: event.markets.map((market) => ({
      id: market.id,
      name: market.name.trim(),
      selections: market.runners.map((selection) => ({
        id: selection.id,
        name: selection.name.trim()
      }))
    }))
  };
}

// ...

const events = response.events.map(rawEventToEventModel);

You could also do that for the inner maps (i.e. participants, metaTags and markets).

The convenience of arrow functions sometimes encourages peoples into the "I'm only going to use this once so don't need a separate function" mentality, but function 'names' do actually convey important information that make code more readable (less code does not always equate to more readable).

I've also taken the liberty to show that you can use arrow function returns to remove the verbiage of return { /* ... */ }, but you might argue that obscures what's going on to some extent.

\$\endgroup\$
3
  • \$\begingroup\$ I agree, function names are informative and there's often no good reason to do away with them. \$\endgroup\$ Jun 2, 2022 at 11:01
  • \$\begingroup\$ @Dave, great suggestion for removing inner "returns". The code really looks "cleaner" now. \$\endgroup\$ Jun 2, 2022 at 11:10
  • \$\begingroup\$ Thumbs-up for the "arrow function mentality" observation. \$\endgroup\$
    – radarbob
    Jun 5, 2022 at 23:58
1
\$\begingroup\$

I'm wary about this part:

date: dateTime.slice(0, 10),
time: dateTime.slice(11, 19),

This seems like an error-prone and possibly region-dependent way of extracting the date and time from an object. It's also an example of "magic numbers" - that is, hardcoded integers that appear in a line of code with no explanation. If you're using Javascript's built in Date class, you can use its array of parsing functions to extract the date and time separately into region-specific formatting. If it's a roll-your-own situation, you should add functions to your dateTime class to do the same. (But it's still better to just use the JavaScript built in Date class since it handles a lot of stuff under the hood you probably don't want to think about, e.g. time zones, daylight savings, leap years/seconds, regional formatting, etc.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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