trjhtr

This is a technical test from a job interview. The feedback was that although it does as asked, the application structure wasn't great.

As a beginner I'm not entirely sure what's wrong with it and would appreciate a review.

/* --- Get data form API and display it in DOM --- */

// URL to get all launches from SpaceX API
const allLaunchesURL = 'https://api.spacexdata.com/v2/launches/all';

// Get launch data from API
const getLaunchData = async (url) => 
 let response = await fetch(url);

 // Check if response is ok, if not throw an error
 if(!response.ok) 
 throw Error(`Error fetching API, response status: $response.statusText`);
 

 let data = await response.json();
 data = data.slice(-10);
 displayData(data);


getLaunchData(allLaunchesURL);

// Display data on DOM
function displayData(data) 
 const results = document.querySelector('.results');
 data.map(launch => 
 results.innerHTML += `
 <tr>
 <td>$launch.flight_number</td>
 <td>$formatDate(launch.launch_date_utc)</td>
 <td>$launch.rocket.rocket_name</td>
 <td>$checkPastOrFuture(launch.launch_date_utc)
 $checkPastOrFuture(launch.launch_date_utc) === 'Launched' ? ' - ' + launchSuccess(launch) : ''
 </td>
 <td>
 <button class="infoButton" id="$launch.flight_number" data-rocket=$launch.rocket.rocket_id onclick="getFlightDetails(this)">Click</button>
 </td>
 </tr>
 `
 )


// Check if launch date is upcoming or in the past
function checkPastOrFuture(date) 
 let currentDate = new Date();
 let dateToCheck = new Date(date);
 return currentDate < dateToCheck ? 'Upcoming' : 'Launched';


// Check success of launch
function launchSuccess(flight) 
 return flight.launch_success ? 'Successful' : 'Failure'
 

// Format the date
function formatDate(date) 
 const d = new Date(date);
 return d.toUTCString();



/* --- Get more info on individual launch when triggered by button click --- */

// Endpoint stub for API queries
// Individual flight
const flightURL = 'https://api.spacexdata.com/v2/launches?flight_number=';

// Rocket information
const rocketURL = 'https://api.spacexdata.com/v2/rockets/';

// Get flight number from button id
async function getFlightDetails(ele) 

 // Get data for flight 
 let response = await fetch(`$flightURL$ele.id`);

 // Error checking
 if(!response.ok) 
 throw Error(`Error fetching flight details, response status: $response.statusText`);
 

 let data = await response.json();
 displayFlightData(data[0]);

 // Get rocket data
 let rocketResponse = await fetch(`$rocketURL$ele.dataset.rocket`);

 // Error checking
 if(!response.ok) 
 throw Error(`Error fetching rocket details, response status: $response.statusText`);
 

 let rocketData = await rocketResponse.json();
 displayRocketInfo(rocketData);


function displayFlightData(flight) 
 const flightDiv = document.querySelector('.flightDetails');
 if(!flight) 
 flightDiv.innerHTML = `
 <h4>Flight details</h4> 
 <p>Sorry, this flight has no further details</p>
 `;
 return;
 

 flightDiv.innerHTML = `
 <h4>Flight details</h4>
 <p>$flight.details</p>
 <p><strong>Launch site:</strong> $flight.launch_site.site_name_long</p>
 <figure>
 <img src="$flight.links.mission_patch" alt="Flight $flight.flight_number Mission Patch" title="Flight $flight.flight_number Mission Patch">
 <figcaption>Flight $flight.flight_number Mission Patch</figcaption>
 </figure>
 <br>
 `


function displayRocketInfo(rocket) 
 const rocketDiv = document.querySelector('.rocketInfo');
 if(!rocketDiv) 
 rocketDiv.innerHTML = `
 <h4>Rocket Details</h4>
 <p>Sorry, this flight has no further details</p>
 `
 ;
 return;
 
 rocketDiv.innerHTML = `
 <h4>Rocket Details</h4>
 <p><strong>Name:</strong> $rocket.name, <strong>ID:</strong> $rocket.id</p>
 <p><strong>Description:</strong> $rocket.description</p>
 <p><strong>Height:</strong> $rocket.height.meters metres</p>
 <p><strong>Mass:</strong> $rocket.mass.kg kg</p>
 <p><strong>Number of stages:</strong> $rocket.stages</p>

 `

After looking at how the functions are laid out, it looks like getLaunchData not only gets the data, it also calls the function displayData. Perhaps it would be more appropriate for the former function to simply return the data and then have the caller pass the data to the latter function.

You could consider wrapping the functions up into a controller object, or since your code already uses some ecmascript-6 features like arrow functions, move the functions into a class as methods.

I see a few places where elements are fetched by class name (e.g. document.querySelector('.flightDetails')). Presuming that there is only one such element for each of those class names, it would be more appropriate to utilize an id attribute. Then the selector would utilize the hashtag for the id selector (e.g. document.querySelector('#flightDetails')). But actually if only one item is fetched by id, then it is quicker to use document.getElementById() because it "is definitely faster"¹ (see this jsPerf test for comparison).

There are a few places where variables are created using let but then never re-assigned (e.g. response, currentDate, dateToCheck, etc.) so const could be used instead for those.

The use of async / await is nice, as well as the error handling. The template literal usage is good, but could be converted to using Javascript Templating (and refer to this CR post for an example). The existing usage of template literals adds a lot of view-centric logic in with the controller code. You could separate such functionality into a view object and a controller object. Also, while it may be unlikely that the SpaceX API would do this, there is a security risk here with template literals evaluating values from the API response. Refer to this SO answer for more of an explanation of this topic.

¹_{https://www.sitepoint.com/community/t/getelementbyid-vs-queryselector/280663/2}