Simplify javascript redirecting functions logic

Question

I have javascript file which is ugly but I have not idea how to make it better. There are many function for redirecting with different number of parameters. Just few functions are used for other thinks. All this functions will be called from Jquery event. Like this:

$("#btnSomeButtonID").click(function () { SelectPTO(); });

I would like to simplify logic. Do you have any ideas for refactoring? Thank you for your help.

// Require refactoring, all is  public. Need to investigate what is best way.

var pushupdatesList; //public
var pushupdatesListRun = true;//public
var loading = false;//public

// functions with 0 parameters
function StartLoading() {
    if (loading)
        return;

    loading = true;
    $("#loadingbar").show();
}

function StopLoading() {
    loading = false;
    $("#loadingbar").hide();
}

// redirect functions with 0 parameters
function OpenAddCompany() {
    location.href = "/Administration/AddCompany";
}

function OpenAdminTools() {
    StartLoading();
    location.href = "/Administration/GetCompanies";
}

function Logout() {
    StartLoading();
    AbortPushUpdates();
    loading = 0;
    location.href = "/Home/Logoff";
}

function SelectPTO() {
    StartLoading();
    AbortPushUpdates();
    loading = 0;
    location.href = "/Home/SelectEquipmentCollection";
}

function OpenChangePassword() {
    StartLoading();
    loading = 0;
    location.href = "/Home/ChangePassword";
}

// functions with 1 parameter
function OpenGetPTOs(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetEquipmentCollections/" + ID;
}

function OpenAddPTO(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddEquipmentCollection/" + ID;
}

function OpenGetAccountsForCompany(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetAccountsForCompany/" + ID;
}

function OpenAddAccount(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddAccount/" + ID;
}

// functions with 2 parameters
function OpenGetAccountsForPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetAccountsForEquipmentCollection/" + ID + "?CompanyID=" + CompanyID;
}

function OpenAddAccountToPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddAccountToEquipmentCollection/" + ID + "?CompanyID=" + CompanyID;
}

function OpenGetVehiclesForPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetEquipmentForCollection/" + ID + "?CompanyID=" + CompanyID;
}

function OpenAddVehicleToPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddEquipmentToCollection/" + ID + "?CompanyID=" + CompanyID;
}

Danny Harding · Accepted Answer · 2018-03-24 22:25:44Z

The first thing I see which will cut the file at least in half is making a single redirectToPage function that redirects to a given page. You have many copies of the same basic function which currently looks like this

function functionName(params) {
    StartLoading();
    loading = 0;
    location.href = "someURL"
}

When you see a similar block of code like this being repeated multiple times, it's a good time to refactor, (it's debatable, but I would consider refactoring at 2 repetitions, and definitely refactor at 3). Your new function will be

function redirectToPage(url) {
    StartLoading();
    loading = false; // explained below
    location.href = url;
}

You can now erase most of that file, using this one more generic function and passing the urls as arguments.

You are also using loading as a boolean sometimes, and sometimes as an integer. Your code will be easier to reason about if you stick to one. It looks like you're only using loading as a boolean, so I changed loading = 0 to loading = false. You can make this change throughout the rest of the file as well.

This may be enough changes for you. If not, there are a few other functions that you have that redirect to a page but with slightly different functionality, such as calling AbortPushUpdates or not setting the loading variable. You can extend your new redirectToPage function to handle these cases like so

/**
 * Call this function giving the url to redirect to, and other options
 */
function redirectToPage(url, options) {
   if (!options || !options.hasOwnProperty('startLoading') || options.startLoading == true) {
     // default is to run StartLoading()
     StartLoading();
   }

   if (options && options.hasOwnProperty('abortPushUpdates') && options.abortPushUpdates) {
     // default is to NOT call AbortPushUpdates
     AbortPushUpdates();
   }

   if (!options || !options.hasOwnProperty('setLoading') || options.setLoading == true) {
     // default is to set the loading variable
     loading = false;
   }

   // Always redirect
   location.href = url;
 }

You can now use this function in any of the following ways

redirectToPage('/Administration/AddAccount/' + accountID);
redirectToPage('/Administration/AddCompany', {setLoading: false, startLoading: false})
redirectToPage('/Home/SelectEquipmentCollection', {abortPushUpdates: true})

Good luck with your refactor, and let me know if you have any follow up questions!

Thank you, your answer is very helpful :). I will inform you about my progress and if I have some questions. — Raskolnikov
– Raskolnikov, Commented Mar 24, 2018 at 13:56

Stack Exchange Network

Simplify javascript redirecting functions logic

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Simplify javascript redirecting functions logic

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions