Skip to main content
added 189 characters in body
Source Link

But the fundamental issue remains - the HTML layout is completely disconnected from the JavaScript. Having to check to see if a button exists in the first place is something that ideally shouldn't be an issue to worry about, at least in a larger or more professional project. What if you had not 1, but 3 or 5 or 10 elements with handlers on different pages, all of which may or may not exist? The codebase would be more difficult to maintain than it should be.

There are a few solutions for this:

But the fundamental issue remains - the HTML layout is completely disconnected from the JavaScript. Having to check to see if a button exists in the first place is something that ideally shouldn't be an issue to worry about, at least in a larger or more professional project. There are a few solutions for this:

But the fundamental issue remains - the HTML layout is completely disconnected from the JavaScript. Having to check to see if a button exists in the first place is something that ideally shouldn't be an issue to worry about, at least in a larger or more professional project. What if you had not 1, but 3 or 5 or 10 elements with handlers on different pages, all of which may or may not exist? The codebase would be more difficult to maintain than it should be.

There are a few solutions for this:

Source Link

Does this open up the possibility for any buggy behavior that I don't know about?

It's not likely to. The if statement is fine, though it can be made cleaner:

  • Rather than selecting the element twice (once to check to see if it exists, another time to call addEventListener on it), save it in a variable:

    const button = document.getElementById('button01');
    if (button) {
      button.addEventListener('click', newThing);
    }
    
  • Or, if you're writing JS which gets transpiled for production (which, in a professional or larger project, you really should be), use optional chaining:

    document.getElementById('button01')?.addEventListener('click', newThing);
    

But the fundamental issue remains - the HTML layout is completely disconnected from the JavaScript. Having to check to see if a button exists in the first place is something that ideally shouldn't be an issue to worry about, at least in a larger or more professional project. There are a few solutions for this:

  • One option is to have a separate <script> file for pages with the form, eg:
<form id="thingSelection2">
...
</form>
<script src="./thingSelection.js"></script>

where thingSelection.js adds the event listener. But that'll require a separate request to the server, which may be a problem on a large page on HTTP 1.1 - if you have lots of different scripts like this, the sheer number of parallel requests could slow things down, especially for users with high latency. (The HTTP/2 protocol doesn't have issues with additional connections to the same server IIRC)

(You could also inline the script, eg </form><script>// etc</script>but I prefer putting scripts in separate files to permit caching)

  • But if it were me, I'd strongly prefer to fully integrate the creation of the HTML with the event listeners for that HTML, to make it completely inconceivable of one existing without the other, using a framework. For example, with React, you could do something along the lines of:
const Things = () => {
    const [item, setItem] = useState('');
    const [selectedOption, setSelectedOption] = useState('foo');
    const clickHandler = (e) => {
        const fn = options[selectedOption];
        if (fn) {
          setItem(fn());
        }
    };
    return (
        <div>
            <select value={selectedOption} onChange={e => setSelectedOption(e.currentTarget.value)}>
                <option value="foo">foo</option>
                <option value="bar">bar</option>
            </select>
            <button onClick={clickHandler}>click</button>
            <output>{item}</output>
        </div>
    );
};

const options = {
  foo: () => 'foo',
  bar: () => 'bar',
};

const Things = () => {
    const [item, setItem] = React.useState('');
    const [selectedOption, setSelectedOption] = React.useState('foo');
    const clickHandler = (e) => {
        const fn = options[selectedOption];
        if (fn) {
          setItem(fn());
        }
    };
    return (
        <div>
            <select value={selectedOption} onChange={e => setSelectedOption(e.currentTarget.value)}>
                <option value="foo">foo</option>
                <option value="bar">bar</option>
            </select>
            <button onClick={clickHandler}>click</button>
            <output>{item}</output>
        </div>
    );
};
ReactDOM.render(<Things />, document.querySelector('.react'));
<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<div class="react"></div>

Then when you're on a page where the Things are needed, you just render <Things />. With this approach, the HTML its associated JS handlers are all completely self-contained in the Things. There's no need to use selectors that may have unintentional collisions. (For example, going with your original code of document.getElementById('button01'), what if some other completely separate section of the HTML on one of the pages had an element which accidentally used the same ID? Then you'll have problems.)

Using a framework like this is admittedly a lot to learn, but it makes codebases significantly more maintainable. It's well worth it for medium-size or larger projects, IMO.


On a different note, your current code could also be improved a bit:

Prefer selectors Selector strings are usually more easily understood and more flexible than other methods of selecting elements (like getElementsByTagName and document.forms.someFormName.someFormElement). The selector string that matches an element will line up with the CSS selector that styles the element. For example, I'd replace:

document.forms.thingSelection2.type.value

with

document.querySelector('#thingSelection2 [name=type]').value

and

const output = document.getElementsByTagName('output')[0];

with

const output = document.querySelector('output');

(no need to select a collection when you only need the single matching element)

Save the value Instead of selecting and extracting the value twice, write DRY code; put it into a variable:

const { value } = document.querySelector('#thingSelection2 [name=type]');
if (value in options) { // Or, use `!options[value]
  return false;
}