Hey Jonathan, welcome to Code Review!
There's an easy way to turn those two maps into one. I'm going to tell you how, and give you some general tips as a bonus. Buckle up!
You asked how to prevent yourself from repeating your loops, which is a perfect use case for a Fragment!
Consider the following:
<row>
<column>
{data.map(item => /*...*/)}
</column>
<column>
{data.map(item => /*...*/)}
</column>
</row>
Assuming those two maps are over the same data, and use the same parameters, we can just wrap the columns in a fragment in a single loop:
<row>
{data.map(item => (
<React.fragment>
<column>
{item}
</column>
<column>
{item}
</column>
</React.fragment>
))}
</row>
So to answer your question:
{weather[weatherDate].map((weatherData, index) => {
const time = weatherData.validTime.slice(
weatherData.validTime.indexOf("T") + 1,
weatherData.validTime.length - 4
);
if (TIMES_TO_SHOW.indexOf(time) === -1) return null;
return (
<React.Fragment key={"thisWeek-" + index}>
<td className="time">
<p key={"weather-data-" + index}>
{time}
</p>
</td>
<td className="values">
<React.Fragment key={index_}>
Temperature: {weatherData.parameters.t.values}°
Windspeed: {weatherData.parameters.ws.values} m/s
Gusts: {weatherData.parameters.gust.values} m/s
Vind direction: {weatherData.parameters.wd.values}°
</React.Fragment>
</td>
</React.fragment>
);
})}
Okay, bye!
...
Kidding! There's more where that came from.
👀❓ To render or not to render
🕳️ Skipping unwanted values
When mapping over an array of values, you might run into values that you don't want to map. When something like that happens, you might want to return null instead of that value -- which is what you've done here.
dates.map((weatherDate, index) => {
if (
weather[weatherDate].length < 4
) {
return null;
}
/* ... */
);
Looking at the code and stepping back a bit, you're putting business logic into your view logic. This makes code more cryptic and thus more difficult to understand. We need to separate "should we render this data?" from "how to render this data?".
Instead of returning null which tells React not to render this data, surely one could just just... not render those values in the array in the first place? This is where our great friend Array.filter comes in. It takes a function that returns a boolean for each element in an array, called a predicate. If that predicate returns true, the element in question is "kept" -- else the element is "discarded":
const numbers = [1,2,3,4,5,6,7,8,9];
const isEven = number => number % 2 === 0;
const evenNumbers = numbers.filter(number => isEven(number));
// > [2, 4, 6, 8]
Side-note: x => y(x) is the same as just y, so the last line could be refactored:
const evenNumbers = numbers.filter(isEven);
How clean is that?!
Applying this newfound knowledge, your code could look like this:
const isValidDate = date => weather[date].length >= 4;
// ...
return dates
.filter(isValidDate)
.map(/*...*/);
In a later mapping, you check if some date is a member of the set DATES_TO_SHOW, and return null if it isn't:
weather[weatherDate].map((weatherTime, _index) => {
const time = weatherTime.validTime.slice(
weatherTime.validTime.indexOf("T") + 1,
weatherTime.validTime.length - 4
);
if (TIMES_TO_SHOW.indexOf(time) === -1) return null;
return (
<p key={"weather-data-" + _index}>
{time}
</p>
);
}
We just learned that we should filter out unwanted items before we render, so let's take a look at how we could solve this:
const weatherToTime = weatherTime => weatherTime.validTime.slice(
weatherTime.validTime.indexOf("T") + 1,
weatherTime.validTime.length - 4
);
const shouldShowTime = time => TIMES_TO_SHOW.includes(time);
// Notice how I used Array.includes instead of .indexOf(x) === -1
// as .includes is much more autological
weather[weatherDate]
.map(weatherToTime)
.filter(shouldShowTime)
.map((time, _index) => (
<p key={"weather-data-" + _index}>
{time}
</p>
);
🍕 Slice it up
Sometimes you just don't care about the first n elements of an array.
dates.map((weatherDate, index) => {
if (
index === 0
) {
return null;
}
/* ... */
);
You could check the index in every iteration and return null in iteration zero, but, again, what if we could skip rendering that item completely?
In some languages there's a function called skip or drop, and in javascript we have Array.slice. Since you used slice in your code, you might already know how it works so you might want to skip over the next codeblock.
With slice you can skip some elements from the start of an array, and optionally drop some from its end:
const digitsAndLetters = [0, 1, 2, 3, "a", "b", "c", "d"];
const letters = digitsAndLetters.slice(4);
// > ["a", "b", "c", "d"]
const digits = digitsAndLetters.slice(0, 4);
// > [0, 1, 2, 3]
So in your case, to skip the first element of dates, we slice 1 item off the start:
dates
.slice(1)
.filter(isValidDate)
.map(/* ... */);
Ooh baby, look at that! 😎 Not only does this just look nicer, it also reduces cognitive load when trying to understand what your code does. Again: separate "what do we render?" and "how do we render?" Separating what and how is actually a great refactoring for all code.
🗝️ Meaningful keys
You really shouldn't be using the map index as a key. Why? In short: React uses keys to identify elements between renders. This means that react assumes keys are stable, meaning the key to some data must always be the same. When using index as a key, you're coupling data to something unrelated (i.e. the iteration count) and thus unstable (sorting your list would change the order of data but not the order of index which might result in render bugs).
In your case it's not directly a concern, but anything that can go wrong, will go wrong.
A simple solution is to use some identifying property of your data. Maybe some property id exists? Or another property that differs from item to item? If you have a date, you could for example .toString() it. If it's a complex object you could hash its contents to get a unique identifying value.
Depending on your data, you should be using one of these approaches (or something similar).
data.map(item => (
<div key={item.id}/>
<div key={item.toString()}/>
<div key={hash(item)}/>
))
🤗 Embrace expressions
You write a lot of arrow functions with blocks, which kind of defeat their purpose. In most languages an arrow function (often called a lambda) is a syntactical construct to take one argument and transform it, returning the result of that transformation. In other words, there is an implicit return when you don't use blocks:
const double = (number) => {
return number * 2;
};
const numbers = [1, 2, 3];
const doubledNumbers = numbers.map(double);
//> [2, 4, 6];
can also be expressed as
const double = number => number * 2;
// Note that parentheses are optional when working
// with a single parameter
const numbers = [1, 2, 3];
const doubledNumbers = numbers.map(double);
//> [2, 4, 6];
The following is a little rant so feel free to skip to the recap.
I don't know why the people in the ECMAScript committee decided blocks should be allowed in arrow functions. We already had function () {}. Because blocks and object literals use the same characters in the grammar, we need to put parentheses around an object in an arrow function:
// You could be fooled into thinking this would return an object
const Person = name => {
name: name,
age: Math.random()
};
// But no, that's a syntax error -- javascript is trying to parse it
// as a block. We need to put parentheses around it to tell javascript
// we're definitely giving it an expression here.
const Person = name => ({
name: name,
age: Math.random()
});
Arrgh!! 😠
🏁 Recap
All right, we've done it! Hopefully you learned a thing or two. The main takeaways are:
