How Can I Refactor This?

Could anyone give me advice on how to refactor this chunk of code? The functions are very similar to each other, they only differ a little bit.

  function offline(streamer){
    let output = `
    <div class="col-xs-12 item">
        <div class="row center middle item-size">
          <div class="col-xs-4 avatar"><img src="https://maxcdn.icons8.com/Share/icon/Messaging//offline1600.png" alt="${streamer}"></div>
          <div class="col-xs-4 content">${streamer}</div>
          <div class="col-xs-4 del"><img class="icon-offline" src="https://images-na.ssl-images-amazon.com/images/I/719zM3RmAxL.png"></div>
        </div>
      </div>`;
    return output;
  }

  function online(streamer, logo, url, status){
    let output = `
         <div class="col-xs-12 item">
            <div class="row center middle item-size">
              <div class="col-xs-4 avatar"><a href="${url}" target="_blank"><img src="${logo}" alt="${streamer}"></a></div>
              <div class="col-xs-4 content"><a href="${url}" target="_blank"><em>${streamer}</em> - <span class="info">${status}</span></a></div>
              <div class="col-xs-4 del"><img class="icon-online" src="https://phpfoxexpert.com/products/forum/uploads/824_1.png"></div>
            </div>
          </div>`;
    return output;
  }

Why do you feel this needs to be refactored?

Since there are a lot of lines that are similar to each other. But I was only curious whether there’s a way to refactor it.

I don’t see too much repetition. You could get rid of the variables and just return the string templates. You could change these to be arrow functions. Other than that, I think it’s more important for these functions to be readable than anything else.

1 Like

The functions are short, do a single thing, and are easy to read. Good job with them, no need to refactor.

Maybe add some comments…

1 Like

I tend to use ES6 whenever I can. In this case, I could have chosen to either put the two functions into an object and do something like that:

app = {
   online(streamer, logo, url, status){
    let output = `
         <div class="col-xs-12 item">
            <div class="row center middle item-size">
              <div class="col-xs-4 avatar"><a href="${url}" target="_blank"><img src="${logo}" alt="${streamer}"></a></div>
              <div class="col-xs-4 content"><a href="${url}" target="_blank"><em>${streamer}</em> - <span class="info">${status}</span></a></div>
              <div class="col-xs-4 del"><img class="icon-online" src="https://phpfoxexpert.com/products/forum/uploads/824_1.png"></div>
            </div>
          </div>`;
    return output;
    }, the other function ...
}

or

 online = (streamer, logo, url, status) => {
    let output = `
         <div class="col-xs-12 item">
            <div class="row center middle item-size">
              <div class="col-xs-4 avatar"><a href="${url}" target="_blank"><img src="${logo}" alt="${streamer}"></a></div>
              <div class="col-xs-4 content"><a href="${url}" target="_blank"><em>${streamer}</em> - <span class="info">${status}</span></a></div>
              <div class="col-xs-4 del"><img class="icon-online"     src="https://phpfoxexpert.com/products/forum/uploads/824_1.png"></div>
                </div>
          </div>`;
return output;
}

But I think it’s easier to use function nameOfFunction() {code} when there are just two functions

I came up with the following refactor, but I think the only thing it really helps is if you made changes in the div class names. Also, it allows you to just call output with one argument (streamer) and the function will “assume” the status is offline. Anyway, I thought I would share what I created.

  function output(streamer, logo="https://maxcdn.icons8.com/Share/icon/Messaging//offline1600.png", url = null, status = null) {
    const onOrOff = status ? "online" : "offline";
    const imageSection = `<img src="${logo}" alt="${streamer}">`;
    const avatarSection = {online: `<a href="${url}" target="_blank">${imageSection}</a>`, offline: imageSection};
    console.log(avatarSection[status])
    const contentSection = {
      online: `<a href="${url}" target="_blank"><em>${streamer}</em> - <span class="info">${status}</span></a>`,
      offline:`${streamer}`
    }; 
    const statusIcon = {
      online: "https://phpfoxexpert.com/products/forum/uploads/824_1.png",
      offline: "https://images-na.ssl-images-amazon.com/images/I/719zM3RmAxL.png"
    };
    return `
         <div class="col-xs-12 item">
            <div class="row center middle item-size">
              <div class="col-xs-4 avatar">${avatarSection[onOrOff]}</div>
              <div class="col-xs-4 content">${contentSection[onOrOff]}</div>
              <div class="col-xs-4 del"><img class="icon-${onOrOff}" src="${statusIcon[onOrOff]}"></div>
            </div>
          </div>    
    `;
  }

If the status icons where contained on your own server where you could change the names to “online.png” and “offline.png”, then you could get rid of the statusIcon object and would clean it up more:

  function output(streamer, logo="https://maxcdn.icons8.com/Share/icon/Messaging//offline1600.png", url = "", status = null) {
    const onOrOff = status ? "online" : "offline";
    const imageSection = `<img src="${logo}" alt="${streamer}">`;
    const avatarSection = {online: `<a href="${url}" target="_blank">${imageSection}</a>`, offline: imageSection};
    const contentSection = {
      online: `<a href="${url}" target="_blank"><em>${streamer}</em> - <span class="info">${status}</span></a>`,
      offline:`${streamer}`
    }; 
    return `
         <div class="col-xs-12 item">
            <div class="row center middle item-size">
              <div class="col-xs-4 avatar">${avatarSection[onOrOff]}</div>
              <div class="col-xs-4 content">${contentSection[onOrOff]}</div>
              <div class="col-xs-4 del"><img class="icon-${onOrOff}" src="http://yourserver.com/images/${onOrOff}.png"></div>
            </div>
          </div>    
    `;
  }

EDIT: One more thing, by using one of the above solutions, you could change your getJSON
from what you current have (below) and get rid of the on and off variables used in your code:

    $.getJSON(URL+streamer, (data) => {
      if(data.stream === null || data.stream === undefined) {
        off = offline(streamer);
        $('#offline').append(off);
      } else {
        const logo   = data.stream.channel.logo,
          channelURL = data.stream.channel.url,
          status     = data.stream.channel.status;
        on = online(streamer, logo, channelURL, status);
        $('#online').append(on);
      }
    });

to something like below:

    $.getJSON(URL+streamer, (data) => {
      if (data.stream === null || data.stream === undefined) {
       $("#offline").append(output(streamer));
      } else {
        const logo   = data.stream.channel.logo,
          channelURL = data.stream.channel.url,
          status = data.stream.channel.status;
        $("#online").append(output(streamer, logo, channelURL, status));
      }
    });
1 Like

:scream: Thank you for having taken your time and refactoring my code. :yum: I’ve only had a glance at it, but I’m going to look at it carefully. I believe I’ll learn some new stuff from it. Cheers!

Eidt: I probably made a mistake when I had decided to use the FlexBox grid library(CSS). Not only made it my CSS huge, but it also forced me to use so many classes, which resembled each other.