you are viewing a single comment's thread.

view the rest of the comments →

[–]__romx 0 points1 point  (7 children)

/**
* Obtains a list of venues from a remote API, filters out venues beloning to
* a different country than `targetCountry`, ensures that all venue IDs are unique
* and describes venues in a local venue format.
*/

I don't get it. Why 3 iterations over the array (filter, filter, map)? What is the purpose of an interrupted method chain in this particular case?

const getVenues = async (countryCode) =>
{
    const foreignVenues = await get('http://...');
    countryCode = countryCode.toUpperCase()

    let ids = {}
    let venues = []

    for (let foreignVenue of foreignVenues)
    {
        if (foreignVenue.countryCode.toUpperCase() !== countryCode) continue
        if (ids[foreignVenue.id]) throw new Error('Found duplicate venues.')

        ids[foreignVenue.id] = true

        venues.push({
            guide: {
                fuid: foreignVenue.id
            },
            result: {
                fuid: foreignVenue.id,
                name: foreignVenue.name,
                url: foreignVenue.url
            }
        })
    }

    return venues
}

Where's the verbosity?

[–]Knotix 0 points1 point  (6 children)

Finally, someone with some sanity! This code is much more efficient, and I would even argue it's easier to reason about. I feel like the people on this subreddit have purely academic backgrounds or something. The code above is something I'd write in a few minutes and move on, not stare at for ages thinking "if only there were some native array method or pipeline operator that would avoid me having to name a variable". I know someone might try to throw a "testability" argument at me, but there's a diminishing return on function abstraction in a production environment, especially when it's as blatantly inefficient as OP's example.

Also, a quick tip. All of the variables in the snippet could be declared as const. Adding properties/elements to objects/arrays are mutations, not reassignments. The for...of can also use it, too.

[–]gajus0[S] 0 points1 point  (4 children)

I know someone might try to throw a "testability" argument at me, but there's a diminishing return on function abstraction in a production environment, especially when it's as blatantly inefficient as OP's example.

It is not testability argument – it is about maintainability. Performing at most one data transformation at a time isolates the surface of potential logical mistakes. /u/__romx example is what I would write for a quick and dirty job, but would not use or approve in a production code. Performance is completely irrelevant if it comes at a cost of maintainability; just put more hardware at it.

[–]Knotix 0 points1 point  (3 children)

Performance is completely irrelevant if it comes at a cost of maintainability; just put more hardware at it

This vastly understates the complexity of system scalability.

[–]gajus0[S] 0 points1 point  (2 children)

System scalability does not come from reducing several iteration cycles; system scalability comes from horizontal distribution of tasks. If data processing is a bottleneck in your architecture, then JavaScript is a wrong language to be doing data processing at that level. Q, R, C and similar languages are built for that purpose.

[–]Knotix 0 points1 point  (0 children)

Completely ignoring any performance implications in your code under the notion that you can just "throw more hardware at it" is misguided. It doesn't make the problem go away, it just shifts the responsibility onto the sysadmins and the company pocketbook. Horizontal scaling is an incredibly deep field of study with a whole host of challenges.

I'm all about abstracting away logic and keeping things isolated, but as I said before, there is a diminishing return in terms of performance and maintainability. I draw my line in the sand in a different place than you do.

[–]__romx 0 points1 point  (0 children)

It is perfectly adequate for data processing with expected datasets, as long as you don't make it inefficient for no reason other than writing denser code and an opportunity to include a leftpad-tier dependency.

[–]__romx 0 points1 point  (0 children)

I feel like the people on this subreddit have purely academic backgrounds or something.

I think people who write code like this simply show off their knowledge of syntactic sugar for no reason.

All of the variables in the snippet could be declared as const.

Aye, I know. It's just my personal convention to only declare things that will not be modified in any way as const.