2
\$\begingroup\$

I have this array of objects:

const array = [
  {continent: 'Africa', country:'Algeria', year:'2018', value:10},
  {continent: 'Africa', country:'Algeria', year:'2017', value:15},
  {continent: 'Africa', country:'Algeria', year:'2016', value:2},
  {continent: 'Africa', country:'Egypt', year:'2018', value:20},
  {continent: 'Africa', country:'Egypt', year:'2017', value:1},
  {continent: 'Africa', country:'Egypt', year:'2016', value:20},
]

I want a new array with the mean by year:

const result = [
  {continent: 'Africa', country:'', year:'2018', value:15},
  {continent: 'Africa', country:'', year:'2017', value:8},
  {continent: 'Africa', country:'', year:'2016', value:11},
]

This is my code:

const yearsTemp = Array.from(new Set(dataset.map((datum: any) => datum.year)).values())
const years = yearsTemp.map(year => year.toString())
const means = years.map(year => {
  const filtered = filter(dataset, { continent: continent, year: year })
  const mean = filtered.reduce((acc, item) => acc + item.value, 0) / filtered.length
  return {
    continent: dataset[0].continent,
    country: '',
    year: year,
    value: mean,
  }
})

Is there a better way?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Is this actually typescript? it doesn't look like valid (vanilla) javascript. And where is filter defined? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Feb 4 at 23:34
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Yeas, it's Typescript. filter is a lodash function (lodash.com/docs/4.17.11#filter). \$\endgroup\$ – sleepwalking Feb 5 at 6:38
2
\$\begingroup\$

Since this is labelled TypeScript, I'm going to review it as such. First of all, you aren't using types, which defeats the purpose of using TypeScript. You should define what your objects look like

interface Datum { //not sure what to call it
    continent: string,
    country: string,
    year: string,
    value: number
}

I'm not a TypeScript expert or anything, but even as a beginner, I'd expect something at least like this. With this, we get that the year is apparently a string. Maybe it's not an issue but it's something to note. Before defining the type I just glanced at the object and assumed it was a number.

Which means that your array declaration should now look like this

const array : Array<Datum> = [
  {continent: 'Africa', country:'Algeria', year:'2018', value:10},
  {continent: 'Africa', country:'Algeria', year:'2017', value:15},
  {continent: 'Africa', country:'Algeria', year:'2016', value:2},
  {continent: 'Africa', country:'Egypt', year:'2018', value:20},
  {continent: 'Africa', country:'Egypt', year:'2017', value:1},
  {continent: 'Africa', country:'Egypt', year:'2016', value:20},
]

and the compiler would know what you are working with providing type safety for future operations.

Now, let's take a look at some other declarations

const yearsTemp : Array<string> = Array.from(new Set(dataset.map((datum: Datum) => datum.year)).values())

Assuming dataset has a content similar to array in the beginning, the types we annotate are Datum for each variable. This means that we are mapping each year value and get an array of string. However, calling .values() on the Set is superfluous - Array.from() works fine with just being passed a Set. So, you can drop that.

But what about the next line

const years: Array<string> = yearsTemp.map(year => year.toString())

So, we take an array of strings and produce an array of strings. That's fine normally, but to do that you call .toString() on each value. That's a pointless operation "some value".toString() is the same thing. It's also dangerous, as null.toString() would cause an error. For future reference, a way that wouldn't throw a null pointer exception is to pass the value through the String constructor function but don't use the keyword new.

let numericValue = 42;

let stringValue = String(numericValue);

console.log("typeof stringValue", typeof stringValue);
console.log('stringValue === "42"', stringValue === "42");

//this creates a new object string, not a string primitive
let dontDoThis = new String(numericValue);

console.log("typeof dontDoThis", typeof dontDoThis);
console.log('dontDoThis === "42"', dontDoThis === "42"); //not the same type
console.log('dontDoThis === new String("42")', dontDoThis === new String("42")); //not the same object

Passing the value null or undefined through String will not cause an error but it will produce the string "null" or "undefined" respectively. You can filter those out, if you don't want them

let objectsWithNumericValues = [{year: 2018}, {year: 2019}, {year: 2019}, {year: 2020}, {year: null}];

let extractedYears = objectsWithNumericValues.map(obj => obj.year);
let resultWithNull = Array.from(new Set(extractedYears))
  .map(String); //<-- convert to string

let resultWithoutNull = Array.from(new Set(extractedYears))
  .filter(Boolean) //<-- filter anything falsey
  .map(String);
  
console.log(resultWithNull);
console.log(resultWithoutNull);

On a separate note, the above code shows how you can avoid temporary variables by chaining operations. It saves some typing and it's clearer by not having clutter like throw-away variable names. But it also depends on the coding style.

Anyway, back on the topic of getting the years and turning them into strings - just drop the line that does toString and rename const yearsTemp to const years

As for the rest it seems OK, as long as you add the types. It's not clear where the continent variable is coming from when you call filter(dataset, { continent: continent, year: year }) but I'd assume you have that in context. The other strange this is dataset[0].continent. I'm not sure if that's correct or not, as it would take the same continent for each new produced value and it's not necessarily going to be the correct one unless dataset[0].continent === continent. But that might be a typo when transcribing the code here. Perhaps the same is true for country: ''

The only change I'd make is separate the functionality instead of put all the processing into .map(). The chaining of array operation makes the separation of intentions clearer and if you need to extract functionality, it's cleaner.

years
    .map(year => filter(dataset, { continent: continent, year: year }))
    .map(filtered => {
        const mean = filtered.reduce((acc, item) => acc + item.value, 0) / filtered.length
        return {
            continent: dataset[0].continent,
            country: '',
            year: year,
            value: mean
        }
    })

this way if you decide that either or both functionalities are reusable, you can separate them and your logic turns into

years
    .map(getResultsWithTheSameYear)
    .map(calculateMean)

and you can use these functions elsewhere. Perhaps you will have some other functionality where you do

countries
    .map(getResultsWithSameCountry)
    .map(calculateMean)
\$\endgroup\$
0
\$\begingroup\$

Review points

Good use of the const keyword for values that are not re-assigned. That is refreshing to see.

Is the empty country property necessary?

Addressing your question

Is there a better way

I’d question what “better” in this case means to you- shorter, simpler, more performant?

Because you mentioned you are using lodash, the _.meanBy() method could be used, along with _.map().

One way would be to group items by year and continent by concatenating those values.

const array = [
  {continent: 'Africa', country:'Algeria', year:'2018', value:10},
  {continent: 'Africa', country:'Algeria', year:'2017', value:15},
  {continent: 'Africa', country:'Algeria', year:'2016', value:2},
  {continent: 'Africa', country:'Egypt', year:'2018', value:20},
  {continent: 'Africa', country:'Egypt', year:'2017', value:1},
  {continent: 'Africa', country:'Egypt', year:'2016', value:20},
];
const groupedItems = _.groupBy(array, record => record.continent + '_' +  record.year);
const means = _.map(groupedItems, (group, key) => {
  return {
    continent: group[0].continent,
    country: '',
    year: group[0].year,
    value: _.meanBy(group, 'value')
  };
});

console.log('output: ', means);
<script src="https://cdn.jsdelivr.net/lodash/4/lodash.min.js"></script>

The key property could also be used to fetch the values for the continent and year of each group - perhaps using array destructuringSection and .

const array = [
  {continent: 'Africa', country:'Algeria', year:'2018', value:10},
  {continent: 'Africa', country:'Algeria', year:'2017', value:15},
  {continent: 'Africa', country:'Algeria', year:'2016', value:2},
  {continent: 'Africa', country:'Egypt', year:'2018', value:20},
  {continent: 'Africa', country:'Egypt', year:'2017', value:1},
  {continent: 'Africa', country:'Egypt', year:'2016', value:20},
];
const groupedItems = _.groupBy(array, record => record.continent + '_' + record.year);
const means = _.map(groupedItems, (group, key) => {
  [continent, year] = key.split('_');
  return {
    continent,
    country: '',
    year,
    value: _.meanBy(group, 'value')
  };
});

console.log('output: ', means);
<script src="https://cdn.jsdelivr.net/lodash/4/lodash.min.js"></script>

\$\endgroup\$

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.