The regular expression cookie hammer

I am guilty of it, you are guilty of it; writing a piece of code that makes someone want to cry. This time I am thankful it is not my code.

The problem we are trying to solve this time is as follows:

There is a <div> with content that scrolls vertically.  When a postback happens, the content in the <div> will be refreshed and the scroll position is reset to the top (scrollTop becomes 0).  The scroll position should not jump around, but rather should remain in the same position.

A solution submitted for your disapproval:

function LoadScrollPosition(scrollableDiv, isPostBack) {
 if (false == isPostBack) {
  ResetScrollPosition(scrollableDiv);
  SaveScrollPosition(scrollableDiv);
 }
 else {
  RestoreScrollPosition(scrollableDiv);
 }
}
 
function ResetScrollPosition(scrollableDiv) {
 if (null != scrollableDiv) {
  // set position
  scrollableDiv.scrollTop = 0;
 }
 document.cookie = "yScrollPos=0";
}
 
function SaveScrollPosition(scrollableDiv) {
 if (null != scrollableDiv) {
  // save the scroll position
  document.cookie = "yScrollPos=" + scrollableDiv.scrollTop;
 }
}
 
function RestoreScrollPosition(scrollableDiv) {
 if (null != scrollableDiv) {
  // get scroll position
  var yScrollPosition = GetCookie("yScrollPos");
  if (yScrollPosition) {
   // restore scroll position
   scrollableDiv.scrollTop = yScrollPosition;
  }
  else {
   // reset scroll position
   scrollableDiv.scrollTop = 0;
  }
 }
}
 
function GetCookie(cookieName) {
 // matches 4 patterns
 // 0th: entire pattern of "cookieName=value;"
 // 1st: beginning of line, or "; " cookie separator--does not capture, as indicated by "?:"
 // 2nd: "(.*?)" -- capture a non-greedy wildcard match
 // 3rd: ";" or end of line--does not capture, as indicated by "?:"
 var patternString = "(?:^|;)\\s*" + cookieName + "=(.*?)(?:;|$)";
 var caseInsensitive = "i";
 var pattern = new RegExp(patternString, caseInsensitive);
 var matches = document.cookie.match(pattern);
 var value = null;
 if ((matches) && (2 == matches.length)) {
  value = matches[1];
 }
 return value;
}

Where to begin? My mother told me, "If you can't say anything nice, don't say anything at all." so maybe it has some merits. Lets see:

• It is in it's own .js file so it can be reused.
• There is some, though mostly useless, comments that document whats going on
• Surprisingly, it does work

Yeah that is about it. I can't really come up with anything else. The fact that I can find anything redeeming about this code is surely a testament for my canonization. Now that I've said something good lets back the cart up and run it over.

I'll just throw out a few of the quick objections and improvements:

• There is no need to check if this is a postback. The objective is to only do this on postback so we should only be calling this method when we are beginning a postback
• The repeated null checks are not necessary and can be pulled out to a higher level
• The removed null checks collapse those functions into 1 line functions that can be pulled out to a higher level
• The reset scroll position is not necessary at all, in fact we specifically never have a need to reset the position because again, this is only supposed to happen during postbacks.

So before we even dig into anything we can distill the mess down to only the required parts:

function SaveScrollPosition(scrollableDiv) {
  document.cookie = "yScrollPos=" + scrollableDiv.scrollTop;
}
function RestoreScrollPosition(scrollableDiv) {
  var yScrollPosition = GetCookie("yScrollPos");
  if (yScrollPosition) {
   scrollableDiv.scrollTop = yScrollPosition;
  }
}
 
function GetCookie(cookieName) {
 // matches 4 patterns
 // 0th: entire pattern of "cookieName=value;"
 // 1st: beginning of line, or "; " cookie separator--does not capture, as indicated by "?:"
 // 2nd: "(.*?)" -- capture a non-greedy wildcard match
 // 3rd: ";" or end of line--does not capture, as indicated by "?:"
 var patternString = "(?:^|;)\\s*" + cookieName + "=(.*?)(?:;|$)";
 var caseInsensitive = "i";
 var pattern = new RegExp(patternString, caseInsensitive);
 var matches = document.cookie.match(pattern);
 var value = null;
 if ((matches) && (2 == matches.length)) {
  value = matches[1];
 }
 return value;
}

Whats left of the code is smaller and more concise. Still ugly, but more manageable. The meat of this approach is based around cookies and regular expressions.

Regular expressions are expressive and powerful, but they are a dual edged sword. They can also be hard to read, hard to build and even harder to debug. When I find myself having to resort to a regular expression to solve a problem, I back up several steps and try to figure out f they are really necessary. One must always carefully weigh the amount of obfuscation brought with complicated regular expressions against clarity and the ability to maintain and debug code.

In the case of the GetCookie function we find a function that is hard to deduce for most programmers. The coder placed some comments, but the comments do not explain what is going on. The comments note that the regular expression matches 4 patterns. Which lead me to more questions:

• Why are there 4 matches?
• What do the 4 matches mean?
• Do all the matches mean the same thing?
• If the expression has 4 matches why do we only look for there to be match length of 2?
• Why do we only ever take the second match?

When I asked the coder these questions, the coder had no real answers. In fact the coder was unsure about anything that was going on with this bit of code. The truly scary part of this is that the coder is in the 5-10 years experience of web programming. For me, under these circumstances, this goes further than code smell. This is not a learning experience or even one of those "Oops" moments we all have. If this is something that you have not "learned" over the years, then what else is going on in your office? Do we need to have a talk about this type of code and why its bad? What purpose will it serve; will you learn and remember next time?

Perhaps a better implementation might have been one where we wired into the framework and and save the position to local memory instead of to a cookie. I might also get a bit more clever and namespace my methods as well as allowing multiple elements to have their positions remembered on the page.

Submitted, hopefully for your approval:

var PostbackActions = PostbackActions || [];
PostbackActions.ScrollPosition = PostbackActions.ScrollPosition || {};
PostbackActions.ScrollPosition.TargetElements = {};
PostbackActions.ScrollPosition.TargetElementPositions = {};
PostbackActions.ScrollPosition.AddTargetElement = function (targetElementId) {
 var element = document.getElementById(targetElementId);
 this.TargetElements.push(element);
};
PostbackActions.ScrollPosition.RemoveTargetElement = function (targetElementId) {
 //remove the element from the list and return the element
};
PostbackActions.ScrollPosition.SaveTargetPositions = function () {
 for (var i = 0; i < this.TargetElements.length; i++) {
  this.TargetElementPositions.push(this.TargetElements[i].scrollTop);
 }
};
PostbackActions.ScrollPosition.RestoreTargetPositions = function () {
 //use shift to remove elements and restore positions
}; 
 
if (typeof (Sys) !== 'undefined') {
 var sysInstance = Sys.WebForms.PageRequestManager.getInstance();
 sysInstance.add_beginRequest(PostbackActions.ScrollPosition.SaveTargetPositions);
 sysInstance.add_endRequest(PostbackActions.ScrollPosition.RestoreTargetPositions);
}