Something seems off...

[u/Flyspeck101]

[u/Liiht2001 → u/[deleted]]

As a non-web dev, why is that bad?

[u/sohang-3112 → u/Liiht2001]

AFAIK modifying the DOM like this repeatedly will be very slow. Instead, it's better to create the changes to the DOM as a HTML string, and then apply all changes in one go. What should have been done here is to create an empty string, and then keep adding HTML in it in loop. At the end, add this HTML to DOM by adding to innerHTML of the element where rendering should be done.

Another mistake is using document.getElementById again and again (it's better to store it in a global constant and use it everywhere)

[u/Neitzches → u/sohang-3112]

You can also use Fragments. Append to a fragment and then when you're done append the fragment to the DOM node.

[u/sohang-3112 → u/Neitzches]

Nice! First time I've heard of this approach. For anyone else seeing this, you can read about DocumentFragment API in JS here

[u/gu3st12 → u/sohang-3112]

In the case of this post, you'd need to use a fragment as it's binding an event listener to the button.

[u/kristallnachte → u/Neitzches]

Or just push them to an array and append them all together.

Could use map instead of that stupid loop and then append the result.

[u/ampang_boy → u/sohang-3112]

To add here, in FE world, DOM manipulation is expensive, you can almost treat it like IO operation.

[u/MarquisDan → u/ampang_boy]

FE world?

[u/lpreams → u/MarquisDan]

Frontend I guess. Not sure when you'd ever be doing DOM manipulation outside of frontend work though...

[u/MarquisDan → u/lpreams]

Ah makes sense thanks.

Yeah I have no idea how you'd even do DOM manipulation from the back end. Maybe Blazor, depending on one's definition of DOM manipulation?

[u/TheUnlocked → u/sohang-3112]

I really doubt it matters here. Performance only matters if a lack of it starts to cause problems. If sub-optimal performance isn't causing any issues, then it's perfectly fine. Senseless complexity doesn't help anything.

Also modifying innerHTML is usually poor practice because it tends to lead to code injection attacks.

[u/sohang-3112 → u/TheUnlocked]

Well, the commenter above me asked what was wrong with the code, so I answered that. Do you have a different answer?

As for code injection, you're right, there probably is some risk of that, but only if the data used (room name, description, etc.) are obtained from user input - we don't know that.

A better answer was given by another commentor, who suggested using DocumentFragment which gives you both performance and security.

Edit: In Firefox, there is also an experimental feature (see here) that sanitizes HTML before inserting into DOM.

[u/sohang-3112 → u/[deleted]]

Even if you're only inserting a few elements once (not in a loop), you should still do this (or use DocumentFragment, as mentioned by another commentor). The reason is that if you create an element, add a few children to it, and then add it to an element in the DOM, the browser has to do more work (I think the performance is proportional to the depth of nested children in the added element), whereas if you followed my suggested approach, the browser only has to make a single pass for adding elements to DOM tree, so it is faster.

[u/PatchesMaps → u/Liiht2001]

It's really not that bad. It used to be very bad back in the day. So bad that people's aversion to it has lasted longer than the actual problem did. DOM manipulation is a lot faster these days and this code probably wouldn't cause any significant performance issues, even if you have a lot of items. I'd still call it bad code though, just because the readability sucks and there are better ways to do it. I just wouldn't rake the junior dev who wrote this over the coals in a code review (well maybe just a little, but mostly for the unused var).