Communities

Writing
Writing
Codidact Meta
Codidact Meta
The Great Outdoors
The Great Outdoors
Photography & Video
Photography & Video
Scientific Speculation
Scientific Speculation
Cooking
Cooking
Electrical Engineering
Electrical Engineering
Judaism
Judaism
Languages & Linguistics
Languages & Linguistics
Software Development
Software Development
Mathematics
Mathematics
Christianity
Christianity
Code Golf
Code Golf
Music
Music
Physics
Physics
Linux Systems
Linux Systems
Power Users
Power Users
Tabletop RPGs
Tabletop RPGs
Community Proposals
Community Proposals
tag:snake search within a tag
answers:0 unanswered questions
user:xxxx search by author id
score:0.5 posts with 0.5+ score
"snake oil" exact phrase
votes:4 posts with 4+ votes
created:<1w created < 1 week ago
post_type:xxxx type of post
Search help
Notifications
Mark all as read See all your notifications »
Code Reviews

Welcome to Software Development on Codidact!

Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.

Vanilla JS Functions Review

+4
−0

I wanted to create a simple static website that would use JS to randomly rotate through a quotes.json file I maintain. My JavaScript experience is more limited as it's not what I do at work, so what I'm seeking is good JS practices and writing in a way that is "idiomatic" to the JS community.

My other goals are to write it in a way where I can have decent test coverage and also avoid having to install dependencies, I'm using DigitalOcean's App Platform which allows for 3 free static sites. The goal is to not introduce anything that would require me to pay for the service.

const quoteText = document.getElementById("quote-text");
const quoteAuthor = document.getElementById("quote-author");
const quoteCitation = document.getElementById("quote-citation");

export async function fetchQuotes() {
    const response = await fetch('data/quotes.json');
    if (!response.ok) {
        throw new Error(`Server returned ${response.status} ${response.statusText}`);
    }
    const data = await response.json();
    if (!Array.isArray(data.quotes)) {
        throw new Error('Invalid data structure: missing quotes array');
    }
    return data.quotes;
}

export function getRandomQuote(quotes) {
    const randomIndex = Math.floor(Math.random() * quotes.length);
    return quotes[randomIndex];
}

function displayQuote(quoteObj) {
    quoteText.textContent = quoteObj.quote;
    quoteAuthor.textContent = quoteObj.author;
    quoteCitation.textContent = quoteObj.citation;
}

function scaleText() {
    const containerWidth = document.querySelector(".quote-container").clientWidth;
    const textWidth = quoteText.clientWidth;
    const scaleFactor = containerWidth / textWidth;

    if (scaleFactor < 1) {
        quoteText.style.transform = `scale(${scaleFactor})`;
    } else {
        quoteText.style.transform = "scale(1)";
    }
}

async function populateQuote() {
    try {
        const quotes = await fetchQuotes();
        const randomQuote = getRandomQuote(quotes);
        displayQuote(randomQuote);
        requestAnimationFrame(scaleText);
        window.addEventListener("resize", scaleText);
    } catch (error) {
        console.error(`Error loading quotes: ${error.message}`);
    }
}

// Load the initial quote
populateQuote();

// Listen for click events on the "New Quote" button after DOM has loaded
document.addEventListener('DOMContentLoaded', () => {
    document.querySelector('.btn').addEventListener('click', populateQuote);
})
History
Why does this post require moderator attention?
You might want to add some details to your flag.
Why should this post be closed?

0 comment threads

1 answer

+3
−0

Without seeing the HTML and CSS for the page I'm going to be making assumptions about the effects that the JavaScript has. Feel free to include the other files if the lack of context leads me to incorrect conclusions.

const quoteText = document.getElementById("quote-text");
const quoteAuthor = document.getElementById("quote-author");
const quoteCitation = document.getElementById("quote-citation");

I like the use of const here, and throughout the file. I find the code much easier to reason about when I don't have to wonder whether I might have overlooked a change in value of a variable.


Warning: const only prevents overwriting a variable. It does not make objects immutable.

Example

const a = 3;
a = 4    // TypeError: invalid assignment to const 'a'

const b = {fruit: "apple"}
b = {fruit: "pear"}    // TypeError: invalid assignment to const 'b'
b.fruit = "pear"    // No error: b == {fruit: "pear"}

export async function fetchQuotes() {
    const response = await fetch('data/quotes.json');
    if (!response.ok) {
        throw new Error(`Server returned ${response.status} ${response.statusText}`);
    }
    const data = await response.json();
    if (!Array.isArray(data.quotes)) {
        throw new Error('Invalid data structure: missing quotes array');
    }
    return data.quotes;
}

Without seeing the HTML, I can't tell what will be displayed to a user on a slow connection while they wait for this function to return, so maybe you have already addressed this. If not, the developer window of most browsers will have an option to simulate a slow connection, making the function take several seconds to return. This will allow you to see what the page looks like during loading the quotes and decide whether there's anything you'd like to be different.

export function getRandomQuote(quotes) {
    const randomIndex = Math.floor(Math.random() * quotes.length);
    return quotes[randomIndex];
}

The previous 2 functions are prefixed export. Is this because they also need to be called from another module? If not, the redundant use of export won't break anything, but it may make future readers of the code wonder if there is more complexity that they've missed.

function displayQuote(quoteObj) {
    quoteText.textContent = quoteObj.quote;
    quoteAuthor.textContent = quoteObj.author;
    quoteCitation.textContent = quoteObj.citation;
}

function scaleText() {
    const containerWidth = document.querySelector(".quote-container").clientWidth;
    const textWidth = quoteText.clientWidth;
    const scaleFactor = containerWidth / textWidth;

    if (scaleFactor < 1) {
        quoteText.style.transform = `scale(${scaleFactor})`;
    } else {
        quoteText.style.transform = "scale(1)";
    }
}

Rather than repeat quoteText.style.transform = in both the if block and the else block, I would probably use the minimum of the scaleFactor and 1 (which will have exactly the same effect). That's just my personal preference - which style a particular person finds more readable will depend on what they are used to.

So instead of

    if (scaleFactor < 1) {
        quoteText.style.transform = `scale(${scaleFactor})`;
    } else {
        quoteText.style.transform = "scale(1)";
    }
}

I might write

    quoteText.style.transform = `scale(${Math.min(scaleFactor, 1)})`;

Is it your intention to reduce the text size rather than wrapping the text onto the next line? It's worth narrowing the window as far as it will go to see how small the text becomes, and whether it is still readable.

If you decide to instead keep the text size constant and change the width of the text container, you can use .style.width instead of .style.transform. In this case you may also want to increase the height to allow for the text wrapping onto more lines.

Whichever approach you choose (shrinking the text or wrapping it onto the next line), it's worth considering doing this with CSS instead of JavaScript. This will allow the browser to handle adjusting the size automatically, rather than you having to listen for resize events from JavaScript. Although JavaScript now runs much faster than in the past, you may still find that using CSS results in smoother changes as you resize the window, particularly for anyone accessing the site from a slower machine (but test both rather than trusting my assumption).

You may even find the code is simpler in CSS as it is designed for this kind of purpose.

For example, to make an element usually 100 pixels wide but never more than 80% of the viewport width, you could set both a width and a max-width:

#element-id {
    width: 100px;
    max-width: 80vw;
}
async function populateQuote() {
    try {
        const quotes = await fetchQuotes();
        const randomQuote = getRandomQuote(quotes);
        displayQuote(randomQuote);
        requestAnimationFrame(scaleText);
        window.addEventListener("resize", scaleText);
    } catch (error) {
        console.error(`Error loading quotes: ${error.message}`);
    }
}

The console.error will be useful for a technical user who knows where to look, but non-technical users may be left wondering whether the page is broken or just slow. Without access to the HTML I don't know what the page looks like when the loading fails. It's worth considering updating the quoteText field with a message about the failure for non-technical users, in addition to the full error message in the console.

You could also send the whole error to the console instead of just the message:

        console.error(error);

instead of

        console.error(`Error loading quotes: ${error.message}`);

This will display in the console as an object that can be expanded to show all its properties, rather than just the message property. In the simple case where message is the only property this is no advantage, but I mention it in case you later decide to add more complex error types.

// Load the initial quote
populateQuote();

// Listen for click events on the "New Quote" button after DOM has loaded

On a small page the gap between the button first being visible and the event listener being added is likely to be too small to matter, but in general you can avoid any user confusion by making the button default to disabled in the HTML, and only enable it after the event listener has been added. In addition to avoiding a confusingly unresponsive button during a slow load, this also means that the button will be disabled for people who have JavaScript disabled, or if an error prevents your JavaScript from running. You could also include a message in the HTML that states that JavaScript is required for this page, and then use JavaScript to remove the message so it only remains visible for users that have JavaScript disabled.

document.addEventListener('DOMContentLoaded', () => {
    document.querySelector('.btn').addEventListener('click', populateQuote);
})

I assume at this point the page is fairly simple and only has one button with class btn, so this will work fine. However, if at any point in future you (or anyone else) adds another button somewhere on the page and happens to give it the class btn, querySelector will only apply this event listener to one of them, and it may not happen to be the one you want. I would be inclined to use an id rather than a class so you don't have to think about this for any future changes.

If you prefer to use a class (perhaps because you want the option of having more than one button that gets a new quote), I'd recommend a more semantic class name such as new-quote-button. You would also need to use querySelectorAll to return all of the elements with that class rather than just one, and then iterate over them to add the event listener to each one.

Paying for static sites

Separately from the review, since you mentioned wanting to avoid paying for static sites:

The only reasons I can think that you might need to pay for static sites are:

  • You want your own domain name
  • You have a huge amount of traffic (many users or very frequent users)
  • You have a very large site

If those don't happen to apply to you, there's no reason you'd need to be limited to just 3 free sites. There are companies that will host your static sites free of charge, provided you don't mind the URL being more like yourname.hostcompany.com rather than yourname.com. I'm not looking to advertise any of them but a quick search will throw up a long list.

History
Why does this post require moderator attention?
You might want to add some details to your flag.

1 comment thread

First off, thank you! This is a really thoughtful and helpful review. I think. My plan is to do more ... (5 comments)

Sign up to answer this question »