berryjen / music-api

1 stars 1 forks source link

Separate out the handler functions so that they are not defined inline #2

Open mrkiley opened 2 years ago

mrkiley commented 2 years ago

Functions are not broken out so that they can be exported and tested. Instead of direction embedding the function definition as part of the router listener call, it would be better to define that function elsewhere, and then reference it at https://github.com/berryjen/music-api/blob/e74e9882530a3b2f81214fe979ab2bd5f7cd1f83/app.js#L17

Here's what that would look like:

function route_handler_for_music_artist(req, res) {
    // Your function's logic would go in here (app.js, lines 18-27)
}

// Reference the previously defined function here
app.get('/music/:artist', route_handler_for_music_artist);

Now you see that you can export that function as part of the broader app.js exported values, and a test file can pick it up. The handlers actually belong in their own module, because you shouldn't actually modify the app variable, since it's created by Express. That module would then export all of the available functions that can be used when the module is consumed. So your code would actually end up looking something like this:

route_handlers.js

let exposed_function = {};

function route_handler_for_music(req, res) {
    res.json(melody);
}

function route_handler_for_music_artist(req, res) {

    // Your function's logic would go here (app.js, lines 18-27)
    var result = melody.Music.find((obj) => {
        return obj.Artist === req.params.artist;
    });

    if (result !== undefined) {
        console.log(result);
        res.json(result);
    }
    else {
        res.status(404).send('Artist not found');
    }
}

exposed_functions['music'] = route_handler_for_music;
exposed_functions['music_artist'] = route_handler_for_music_artist;

module.exports = exposed_functions;

app.js

const RouteHandlerModule = require('route_handlers.js');

app.get('/music', RouteHandlerModule.route_handler_for_music);
app.get('/music/:artist', RouteHandlerModule.route_handler_for_music_artist);

module.exports = app;

So what this will ultimately do is allow you to explicitly test the route handlers module, and all of the exposed functions that it contains. There's no need to test the app.js because it's only doing Express operations that have already been confirmed as "tested" by the official maintainers of the Express package.

Forgetting the testing, the separation of the code into appropriate modules makes everything cleaner, and reusable in the event that the same functions need to get used elsewhere.