0
\$\begingroup\$

I got tasked with writing a page that will add as many profile entries as the user wants, and web programming is not at all my field. There are multiple input groups but I'm just using the "age" one as an example. Currently I'm using a function to create DOM elements, then I assemble them together and return the completed outer HTML code. Is this the correct way to do this? Here's my code:

function addInputGroup(userID, userAge) {
                let ageCol = document.createElement('div');
                ageCol.setAttribute('class', 'col-4');
                
                let ageLabel = document.createElement('label');
                ageLabel.setAttribute('class', 'sr-only');
                ageLabel.innerHTML = "Age";
                
                let ageInputGroup = document.createElement('div');
                ageInputGroup.setAttribute('class', 'input-group');
                
                let ageInputGroupPrepend = document.createElement('div');
                ageInputGroupPrepend.setAttribute('class', 'input-group-prepend');
                
                let ageInputGroupText = document.createElement('div');
                ageInputGroupText.setAttribute('class', 'input-group-text');
                ageInputGroupText.innerHTML = "Age";
                
                let ageInput = document.createElement('input');
                ageInput.setAttribute('id', 'fldAge'+userID);
                ageInput.setAttribute('type', 'text');
                ageInput.setAttribute('class', 'form-control');
                ageInput.setAttribute('placeholder', userAge);
                ageInput.setAttribute('value', userAge);
                
                //assemble age column
                ageInputGroupPrepend.appendChild(ageInputGroupText);
                ageInputGroup.appendChild(ageInputGroupPrepend);
                ageInputGroup.appendChild(ageInput);
                ageCol.appendChild(ageLabel);
                ageCol.appendChild(ageInputGroup);
                
                return(ageCol.outerHTML);
            }

Then I call document.write(addInputGroup("12345", "18")); when I need to create an "age" input group.

Does this look correct?

\$\endgroup\$
3
  • \$\begingroup\$ Welcome to Code Review! If you're not sure whether your code works correctly, I'm afraid it's not yet ready for review. Code Review is for finished code that you believe to be functionally correct. Please read What topics can I ask about here? - note that it states "If you are looking for feedback on a specific working piece of code...then you are in the right place!". \$\endgroup\$ Dec 31 '20 at 18:17
  • \$\begingroup\$ Does this look correct? How have you arranged for testing your code? I see two interpretations for the question I quoted: a) Does this look the correct way to achieve the code's goal? b) Does this look as if it actually achieved the code's goal? - the latter is off-topic. \$\endgroup\$
    – greybeard
    Dec 31 '20 at 21:30
  • \$\begingroup\$ (16 is unusually large a code indent.)(Would I expect imports, say, for bootstrap?) \$\endgroup\$
    – greybeard
    Dec 31 '20 at 21:32
0
\$\begingroup\$

I hope you don't have to do too many of these.

Quick points

  • You don't need to use setAttribute. you can assign directly to the property defined in the DOM eg ageInput.id = 'fldAge' + userID;
  • Why add a placeholder that is identical to the input's value. Should it not say something like "Users age"

Very poor code

You code is near impossible to read.

Names are overwhelmed by repetitive use of verbose DOM calls, I see that you did use names to relate order but in the noise I was too lazy to bother following it.

Structure code

Code that is predominantly data entry should focus on the data (elements / forms / HTML) and should do its upmost to remove as much of the un-associated clutter as possible.

It should be methodical in its ordering. It may not be possible to structure the script to match the data, but some method or logical rules can go a long way.

Use functions

Functions were originally invented to simplify exactly this type of mess. Making life easier for coders

JavaScript makes it exceedingly easy to create functions, use them, it will make coding much less of the chore your question implies it is...

"I got tasked with writing a page..."

Rewrite

The rewrite address the two tasks your code does. It does this using two very simple functions

The two tasks are

  1. Create an populate elements with properties.
  2. Append elements to created elements.

The two function return elements and can thus be nested.

By nesting the function calls you can structure the code to match the result.

The two functions...

const tag = (tag, className, props = {}) => 
    Object.assign(document.createElement(tag), {className, ...props});
const append = (par, ...sibs) => 
    sibs.reduce((p, sib) => (p.appendChild(sib), p), par);

With which you can create code that matches the structure of the HTML and is predominantly data.

function inputGroup(userID, age) {
    return append(tag("div", "col-4"),
        tag("label", "sr-only", {textContent: "Age"}),
        tag("div", "input-group"),                         
        append(tag("div", "input-group-prepend"),
            tag("div", "input-group-text", {textContent: "Age"})),
        tag("input", "form-control", {id: "fldAge" + userID, type: "text", value: age}));
}
append(document.body, inputGroup(userID, userAge));

minus all this unrelated noise.

document.createElement
document.createElement
document.createElement
document.createElement
document.createElement
document.createElement
.setAttribute
.setAttribute
.setAttribute
.setAttribute
.setAttribute
.setAttribute
.setAttribute
.setAttribute
.setAttribute
.setAttribute
.appendChild
.appendChild
.appendChild
.appendChild
.appendChild
ageCol
ageCol
ageCol
ageCol
ageCol
ageLabel
ageLabel
ageLabel
ageLabel
ageInput
ageInput
ageInput
ageInput
ageInput
ageInput
ageInput
ageInputGroup
ageInputGroup
ageInputGroup
ageInputGroup
ageInputGroup
ageInputGroupText 
ageInputGroupText
ageInputGroupText
ageInputGroupText
ageInputGroupPrepend
ageInputGroupPrepend
ageInputGroupPrepend
ageInputGroupPrepend
\$\endgroup\$
5
  • \$\begingroup\$ This is what I was hoping to find, an incredibly efficient way that I didn't know was possible. Thank you for this. I wasn't aware of the ... syntax and found a tutorial on it. If you don't mind 1 more question, I have a button that calls a function to setup the profile which will call functions like the one above to setup name, age, etc. Once the user is done adding entries, there is a save button that will take all entries and write it to a database. Is that an efficient way to achieve dynamic list creation/storage? \$\endgroup\$
    – DeQuantus
    Jan 1 at 0:09
  • \$\begingroup\$ @Jeff2005 If you are referring to the rest syntax ... then maybe, but I cant comment on unknowns. There are many ways to gather and process data in JS what is best suit will change depending on needs. It is best to become familiar with what the language can do. MDN is a good reference, the link is to the spread operator AKA rest syntax '...' The site has good coverage of JS with examples and more.. developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… \$\endgroup\$
    – Blindman67
    Jan 1 at 2:02
  • \$\begingroup\$ Thanks. Is there a way to make the tag function work with hyphenated attributes like 'data-parent'? I tried ['data-parent'] also and it didn't work. \$\endgroup\$
    – DeQuantus
    Jan 2 at 22:10
  • \$\begingroup\$ @Jeff2005 data is a called dataset by the DOM and is an object You can assign to the dataset if you create a function const data = (el, data) => (Object.assign(el.dataset, data), el);. You use it like data(tag("div", "myClass",{textContent:"Some text"}),{parent: "somevalue"}) Note that dataset names are modified by the DOM. You will need to make sure you understand how it works to use it correctly developer.mozilla.org/en-US/docs/Web/API/HTMLOrForeignElement/… \$\endgroup\$
    – Blindman67
    Jan 2 at 22:13
  • \$\begingroup\$ Thank you again. Wish there was a "thanks" or upvote for comment button. \$\endgroup\$
    – DeQuantus
    Jan 2 at 23:50
3
\$\begingroup\$

DOM

The outer HTML conversion is probably unecessary. You can manipulate DOM nodes directly. The appending could look something like this.

// note that the HTML representation is irrelevant; we go straight to the DOM
document.getElementById('input-groups').appendChild(ageCol)

JS

addInputGroup() seems to be a misleading name, as the function creates a new DOM element; it doesn't do anything with it. createInputGroup() would be more appropriate I think.

You can use const instead of let here. This provides some nice guarantees about consistency, and makes these guarantees immediately visible to the reader.

return(ageCol.outerHTML); is more commonly written as return ageCol.outerHTML;

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the help. I'll make the changes you're suggesting. Do you mind looking at my other original question linked below? It was rejected for being an opinion based question, but at this point I need opinions since I don't know if my approach is close to acceptable or very inefficient: stackoverflow.com/questions/65517018/… \$\endgroup\$
    – DeQuantus
    Dec 31 '20 at 21:07
2
\$\begingroup\$

Overall your code is not bad perse, but you can make some improvements to it.

Do not use document.write

document.write has several issues and should not be used. Instead, if you need to dynamically add DOM elements, select the parent element and use something like Node.insertBefore(). For more information, please read Why is document.write considered a “bad practice”?.

Naming functions

Your function does not do what it claims it does. It claims to add an input group, but it is limited to "Age". Make it more generic.

a11y

Accessibility-wise, your code is messy. Your label is not attached to any input. A label should either surround the input field (<label>Label name <input ...></label>) or use the for attribute. This way, screen readers actually know what a field needs to contain, and clicking the label focuses the input field.

Furthermore, you are currently printing "Age" twice; once in a screenreader only field and once normally. Screenreaders would thus read it twice to the user.

To fix this, restructure the rendered html.

Other improvements

All your variables are not changed after declaring them. You can declare them with const instead. return is a statement and not a function. You don't have to call it and can instead write return whateverYouWantToReturn. You can use template strings to clean up strings that are composed of different substrings. Instead of writing 'fldAge'+userID you can write

`fldAge${userId}`

Lastly, consider if you can instead do this on the server-side instead of with javascript. Especially since you use document.write, it does sound like you are not dynamically adding the inputs, and as such you are better off by just returning the html immediatelly instead of doing it in the browser.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.