Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

How can I improve this window timeout alert code? I need to add it to some third-party master page and don't want to add a separate file for it.

It must be JS only and IE-8 supported.

jsFiddle

(function () {
//    timedRefresh(10); //3600000
    delayedAlert();

    var timeoutID;

    function delayedAlert() {
        clearAlert();
      timeoutID = window.setTimeout(slowAlert, 10000);
    }

    function slowAlert() {
      alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }

    function clearAlert() {
      window.clearTimeout(timeoutID);
    }
})();
share|improve this question
    
is this for a server side session timeout? – Malachi Aug 24 '15 at 20:14
    
@Malachi yes, we have TMG server for that – Mathematics Aug 25 '15 at 10:02
    
I don't think you are hosting a website on TMG, are you? when you say Master page I assume you are talking about ASP (Active Server Pages) that make use of Microsoft's .NET framework. why aren't you handling session timeout? – Malachi Aug 25 '15 at 13:16
    
@Malachi following what manager says... we are using SharePoint master page, where i am going to add this code, its a quick fix, we may come back to it later on, but atm it serves the purpose :) – Mathematics Aug 25 '15 at 13:18
    
again, I ask, why aren't you using the session timeout. you are making a random timeout value that doesn't correlate to anything except for a magic number. – Malachi Aug 25 '15 at 13:21
up vote 5 down vote accepted

A few points to make about this:

(function () {
//    timedRefresh(10); //3600000
    delayedAlert();

    var timeoutID;

    function delayedAlert() {
        clearAlert();
      timeoutID = window.setTimeout(slowAlert, 10000);
    }

    function slowAlert() {
      alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }

    function clearAlert() {
      window.clearTimeout(timeoutID);
    }
})();
  • slowAlert/delayedAlert can be improved by moving slowAlert into delayedAlert as follows:
function delayedAlert() {
    clearAlert();
    timeoutID = window.setTimeout(function(){
        alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }, 10000);
}  
  • I can't see why the clearTimeout call deserves its own function, when it only gets called from delayedAlert; You could just put it in delayedAlert:
function delayedAlert() {
    window.clearTimeout(timeoutID);
    timeoutID = window.setTimeout(function(){
        alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }, 10000);
}
  • You can make delayedAlert an anonymous function, so that it cannot be called other than inside that block by changing function delayedAlert() into var delayedAlert = function()

  • You call delayedAlert before timeoutID is defined:

delayedAlert();

var timeoutID;

If you're going to use globals (which are recommended against), declare them first, at the top.

(function(){
    var timeoutID;
    // functions and whatnot
    delayedAlert();

All resulting in:

(function(){
    var timeoutID;
    var delayedAlert = function() {
        window.clearTimeout(timeoutID);
        timeoutID = window.setTimeout(function(){
            alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
        }, 10000);
    }
    delayedAlert();
})();
share|improve this answer

I know that you are writing this for an ASPX page, so I am suggesting that you use the Server's session timeout instead of using a magic number for the timeout.

in this function:

function delayedAlert() {
    clearAlert();
  timeoutID = window.setTimeout(slowAlert, 10000);
}

you could write it like this

function delayedAlert() {
    var sessionTimeout = "<%= Session.Timeout %>";
    clearAlert();
    timeoutID = window.setTimeout(slowAlert, sessionTimeout);
}

this way you won't timeout too early or timeout too late, you will always timeout almost exactly as the session times out.


Extra Information was gathered from a discussion posted in a chat room along with the comments to the OP

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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