55502f40dc8b7c769880b10874abc9d0

Looks very bloated and unorganised. Is there a better way to achieve this? js fiddle is available here: http://jsfiddle.net/844Dv/ for testing

element names or html structure cannot be changed as this is a greasemonkey script acting upon a page

thanks!

var neededChars = [];

function selectChar(index, value)
{
  var input = document.getElementsByName('frmentermemorableinformation1:strEnterMemorableInformation_memInfo' + index);
  if (!input) return;
  
  input = input[0];
  
  var valueIndex = getValueIndex(input, value);
  
  if (valueIndex > -1)
  {
    input.selectedIndex = valueIndex;
  }
}


function getValueIndex(selectElement, findValue)
{
  var i = 0, child, val;
  
  while (child = selectElement.childNodes[i])
  {
    val = child.value.replace(/ /,'');
    if (val == findValue) return i;
    i++;
  }
}

function createPasswordInput()
{
  var div = document.createElement('div');
  
  div.setAttribute('style', 'position: fixed; top: 0; left: 0; height: 20px; width: 100%; padding: 5px; background: #FFE8C1; border-bottom: 1px solid #EEA11C; z-index: 100');
 
  div.innerHTML = '<font size="2"><b>Enter your memorable information password:</b></font> &nbsp;&nbsp; <input type="password" id="memorable_password"/>';
  div.setAttribute('id', 'password_helper');
  
  document.body.appendChild(div);
  
  var body = document.body;
  document.body.insertBefore(div, body.firstChild);
  
  var password_input = document.getElementById('memorable_password');
  
  password_input.addEventListener('keyup', function(event) { onPasswordChange(); }, false);
}

function onPasswordChange(event)
{ 
  var password = document.getElementById('memorable_password').value,
      i = 0;
        
  while (i < 3)
  {
    var c = password[neededChars[i] - 1];
    selectChar((i + 1), (c != undefined ? c : '-'));
    i++;
  }  
}

function removePasswordInput()
{
  var div = document.getElementById('password_helper');
  if (div) delete div;
}

function init()
{
  neededChars = getNeededChars();
  createPasswordInput();
  
  window.addEventListener('unload', removePasswordInput, false);
}

function getNeededChars()
{
  var returnChars = [],
      div = document.getElementsByClassName('formFieldInner')[0],
      charFields = div.getElementsByClassName('clearfix');
  
  for (var i = 0, len = charFields.length; i < len; i++)
  {
    var label = charFields[i].childNodes[0];

    if (label.tagName == 'LABEL')
    {
      var match;
      if (match = label.innerHTML.match(/^Character (\d+):$/))
      {
        returnChars.push(match[1]);
      }
    }
  }
  return returnChars;
}

init();

Refactorings

No refactoring yet !

D41d8cd98f00b204e9800998ecf8427e

lomax

October 1, 2010, October 01, 2010 22:52, permalink

No rating. Login to rate!

> Looks very bloated and unorganised.

I agree with your assessment. So, let's trim the bloat and organize it:

I've encapsulated your logic in a class and arranged it by functionality and alphabetical order, and abstracted out the repetitive bits. Nobody likes it when you pee in the global pool.

I personally avoid declaring multiple variables in comma separated lists, as i find it less legible than individual statements and prone to inadvertent global scope pollution during maintenance phases.

I would also try to refactor things like ids, styles and html out of your class (to facilitate configurational updates), but they're fine for a throwaway specific-use tool like this.

I prefer to use DOM methods when possible, and preserve my own references; this prevents id collision (however unlikely), and saves on lookup time.

Since the intended targets are the select inputs, I've started by finding those, and use them as the reference point to get the labels/character indexes, as well as for the count for the character checks; that seems a tad safer than relying on an unbroken, unchanged html layout.

Your cleanup functionality simply did not work; you were not removing your div, you were creating a reference to it, then deleting that reference; it's also a good policy to clean up any events you've added.

Initially, I had the code in the constructor living in an onload event handler following the same pattern as the window unload path. Clearly that doesn't work for jsFiddle; I have no idea if it works for greasemonkey or not. The code then assumes currently, that the window onload event has already fired, and that the dom is clean and green.

Hopefully this time you'll find my method naming less ..."clumsy"... *koff*

Cheers,

lomax

PasswordHelper=new function(){
    // private members
    var _characterIndexes=[];
    var _characterInputs=[];
    var _events=[];
    var _passwordHelper=null;
    var _passwordInput=null;
    
    // magic string id struct
    var _elementIDs={
        form:"frmentermemorableinformation1",
        inputFormat:"frmentermemorableinformation1:strEnterMemorableInformation_memInfo"
    };

    // ctor
    new function PasswordHelper(){
        _characterInputs=getCharacterInputs();
        _characterIndexes=getCharacterIndexes();
        createPasswordInput();
        addEvent(window,"unload",Window_Unload);
    }
    
    // private methods
    function addEvent(elt,type,handler){
        _events.push({elt:elt,type:type,handler:handler});
        elt.addEventListener(type,handler,false);
    }
    
    function clearEvents(){
        for(var i=0;i<_events.length;i++){
            _events[i].elt.removeEventListener(_events[i].type,_events[i].handler,false);
        }
    }

    function create(parentNode,attributes,type,index){
        var elt=document.createElement(type||"div");
        for(var x in attributes){
            if(x=="style")elt.setAttribute(x,attributes[x]);
            else elt[x]=attributes[x];
        }
        if(parentNode)parentNode.insertBefore(elt,parentNode.childNodes[index]);
        return elt;
    }

    function createPasswordInput(){
        var style='position: fixed; top: 0; left: 0; height: 20px; width: 100%; padding: 5px; background: #FFE8C1; border-bottom: 1px solid #EEA11C; z-index: 100';
        var _passwordHelper=create(document.body,{style:style},"div",0);
        create(_passwordHelper,{innerHTML:'Enter your memorable information password:',style:'font-weight:bold;font-size:small;margin-right:10px;'},"span");
        _passwordInput=create(_passwordHelper,{type:"password"},"input");
        addEvent(_passwordInput,"keyup",PasswordInput_Change);
    }

    function getCharacterInputs(){
        var form=document.getElementById(_elementIDs.form);
        var nodes=[];
        for(var i=0;i<form.elements.length;i++){
            if(form.elements[i].tagName=="SELECT"&&form.elements[i].id.indexOf(_elementIDs.inputFormat)==0){
                nodes.push(form.elements[i]);
            }
        }
        return nodes;
    }

    function getCharacterIndexes(){
        var indexes=[]
        for(var i=0;i<_characterInputs.length;i++){
            var label=_characterInputs[i];
            while(label&&label.tagName!="LABEL"&&label.htmlFor!=_characterInputs[i].id){
                label=label.previousSibling;
            }
            if(label&&label.textContent.match(/^Character (\d+):$/)){
                indexes.push(parseInt(RegExp.$1)-1);
            }else{
                indexes.push(-1);
            }
        }
        return indexes;
    }
    
    function selectCharacter(select,character){
        select.selectedIndex=0;
        select.value='&nbsp;'+character;
    }
    
    // event handlers
    function PasswordInput_Change(e){
        for(var i=0;i<_characterInputs.length;i++){
            selectCharacter(_characterInputs[i],_passwordInput.value.charAt(_characterIndexes[i]));
        }
    }

    function Window_Unload(e){
        clearEvents();
        if(_passwordHelper)_passwordHelper.parentNode.removeChild(_passwordHelper);
        _characterIndexes.length=_characterInputs.length=_events.length=0;
        _characterIndexes=_characterInputs=_events=_passwordHelper=_passwordInput=null;
    }
}

Your refactoring





Format Copy from initial code

or Cancel