Weather App Project -FCC Frontend Challenge

Code Feedback
1

Hi I just finished the weather app in the FCC frontend challenge. Any feedback is appreciated .Here is the preview of the site Codepen

2

First of all, is that still one of the FCC projects? I know it used to be, but I havenā€™t seen one in a while. But OK, letā€™s take a lookā€¦

Next, Iā€™m not getting any data - just a screen that looks like your app, but with no data.

When I look in your dev tools console, I see the error message:

TypeError: Cannot read property ā€˜countryā€™ of undefined
at fetchWeather (pen.js?key=pen.js-f3230cce-7735-03c8-7f47-f265fd848bba:51)

presumable that refers to this line:

          loc: data.name + ", " + data.sys.country,

it is saying that data.sys is not defined. Your data that is coming back is:

{error: "Please provide longitude as lon and latitude as lat as numbers/floats."}

because your location is not getting set, itā€™s just an empty object.

I donā€™t know, if it works for you, it might be some security setting I have. But it should handle that more elegantly, I would think. But maybe that is too much to expect for a learning project.

So, I canā€™t judge the functionality of the app as is. But I can look at the codeā€¦ Keep in mind that I tend to be nitpicky about code reviewsā€¦

const App = () => {
  const ENDPOINT = "https://weather-proxy.freecodecamp.rocks/api/current?";

I would have put that constant up higher, out of the way. Or irl it might be in a different file. I donā€™t need to know what is in the constant, just be able to use the constant.

Constants are one of the few times I like global variables.

const [cels, setCels] = React.useState(true);

I donā€™t like cryptic variables like this. What is a ā€œcelā€? I have to know what the code is doing to know what that means. Plus, I would expect it to be another. I would have called it something like ā€œisCelsiusā€ or ā€œisCelsiusSelectedā€ or ā€œisMetricSelectedā€. When I read that variable name, I know exactly what it is, no guessing. (It is common to prefix boolean variables with things like ā€œisā€, ā€œcouldā€, ā€œhaveā€, or ā€œshouldā€ or something like this for this reason. I also like to start function names with action verbs.)

  const toFahrenheit = (celsius) => {
    return (celsius * 1.8 + 32).toFixed(1);
  };

Another example of something I would have put elsewhere. It is a pure function so we could define this anywhere. I wouldnā€™t crowd up the component. irl, Iā€™d put this in another file. The name is enough to know what it does so Iā€™d just put it at the top or bottom of the file, to get it out of the way.

const position = navigator.geolocation.getCurrentPosition(({ coords }) =>

Why are you saving this variable? You donā€™t use it anywhere. And since your callback returns nothing, I would expect it to be undefined.

<Location {...weather} />

Iā€™m not a big fan of spreading out variables in props like this. Iā€™ve had to work on code where this gets done 12 layers deep (including spreading props) and it gets insane. Send exactly what you want, explicitly. If the component needs all of those values (or even most) then just send the weather object. But as it is, the component only needs one, the loc value (another poor variable name, imho). Or some people just pass the weather object (without spreading) and deconstruct what it needs.

handler={setCels}

I think ā€œhandlerā€ is too vague here. If it was a mini component that only handled just that, like UnitSelection that would be different. But I have to guess what it means without context.

        <div className="temp-icon">
          {/rain/i.test(desc) || /drizzle/i.test(desc) ? (
            <FiCloudRain />
          ) : /cloud/i.test(desc) ? (
            <WiDayCloudyGusts />
          ) : /thunder/i.test(desc) ? (
            <WiThunderstorm />
          ) : /wind/i.test(desc) ? (
            <RiWindyLine />
          ) : /sun/i.test(desc) ? (
            <MdWbSunny />
          ) : (
            <TiWeatherCloudy />
          )}
        </div>

This is messy. I have chained ternaries and I hate too much logic in JSX. I would have extracted this out to a function called, ā€œgetTempIconā€, or maybe made this itā€™s own component.

{temp && (cels ? temp.toFixed(1) : converter(temp))}

Why not have your ā€œconverterā€ called ā€œdisplayTempā€. You could pass in temp and cels. If temp does not exist, then you can just return null (which React will not display). Then you can just call it here with:

  { displayTemp(temp, cels) }

People tend to frontload their logic. But anytime you can push it out of your big main component it makes your main component easier to read and puts it in supporting components or functions, out of the way. You donā€™t need to see that logic every time you look at the main component. Cleaning up the main component is always a good thing. The component Weather doesnā€™t need to know the details of how the temp will be displayed, it can just let something else handle it, delegate that business to something else so it can clearly and cleanly focus on the bigger issue. That is one of the beauties of React.

{max_temp && (cels ? max_temp.toFixed(1) : converter(max_temp))}

And hereā€™s another reason for that last comment - you could reuse the logic. In fact you could just make a component DisplayTemp and pass in those values.

<p>{wind}km/h</p>

If youā€™re going to allow them to select between metric an imperial, why not here too?

  margin-bottom: 0px;

Most people would just write this as:

  margin-bottom: 0;

ā€¦no need for units. And didnā€™t you already preset all the margins to 0? Maybe Iā€™m confused.

OK, but other than that, from what I can see, the page looks nice and the code is reasonably put together. It looks better than my first React projects.

I got nitpicky about variable names and organization and such ā€¦ Readability is a big thing for me. As programs get bigger, the importance for readability grows exponentially. And if you are working on a team, it becomes even more important. You will have to sometimes scan through 1000s of lines of code while trying to understand a problem - every little comprehension speed bump kills you.

Good variable names and logical organization. I always say that good code should read like a story.

But still, good job.

3

Hi Kevin, Thank you for your valuable feedback .

Yes, this a FCC project part of the Legacy Frontend certification.

The reason why you donā€™t get any data is Iā€™m using the geolocation API to fetch your current latitude and longitude . So if you do not allow location access couldnā€™t access the weather info which fetched using the position returned from the geolocation. I will try to add some modal or something to inform the error to the user.

I added the ENDPOINT in the same file since it doesnā€™t hold any api key or something private.

I suck at naming things :slight_smile: .

The reason I reset the margin-bottom is I added a margin for every element inside the root

.weatherapp *{
  margin-bottom:0.5em;
}

so I had to reset it for the elements which donā€™t want a margin.

Once again Thank you for your time. I hope your support in my future projects too. I will keep all your advice in mind for the next ones.

4

Yeah, that makes sense. I assumed it was something like that as this is my work computer.

I added the ENDPOINT in the same file since it doesnā€™t hold any api key or something private.

Sure, I understand that. And it really isnā€™t an option on codepen. But I would have at least moved it out of the flow of the component. The fewer lines a component has, the easier it is to read. Move things out of it that donā€™t need to be exactly there.

I suck at naming things :slight_smile: .

Yeah, itā€™s a skill. There have been plenty of times when my cubicle mate and I turned to each other asking for help in finding a good name. But it really does matter. If you canā€™t come up with a good name for data, then it often means that youā€™re not understanding the data. If you canā€™t come up with a good name for a function, it may be that you donā€™t understand it or it is doing too much or the wrong things.

But it is tough. There are certain rules of thumb, but yeah, it can be tough. There is the old adage:

There are only two hard things in Computer Science: cache invalidation and naming things.

Or the more pithy extension:

There are 2 hard things in Computer Science: cache invalidation, naming things, and off-by-1 errors.

But it is worth it. Well written code is sooooooo much easier to maintain.

The reason I reset the margin-bottom is I added a margin for every element inside the root

OK, I guess I missed that.

1 Like
5

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.