Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a simple web page. The user enters the name and rank of a soldier and hits submit. If the input is invalid, an appropriate error message gets displayed. Otherwise, it adds the soldier to a database and adds it to a list of current Soldiers via AJAX request. Here's my Javascript for that:

//Find elements on page
var rankField = $("input[name='rank']");
var nameField = $("input[name='name']");
var submit = $("input[type='submit']");
var list = $("ul");

//attach event-handler to submit button
submit.click(function() {
  var rank = rankField.val();
  var name = nameField.val();
  onSoldierSubmitted(rank, name);
});

function onSoldierSubmitted(rank, name) {
  clearErrors();

  var rankIsValid = isValidRank(rank);
  var hasErrors = false;

  if (!rankIsValid) {
    addError('rank is invalid');
    hasErrors = true;
  };

  if (!name) {
    addError('Please enter a name for your Soldier');
    hasErrors = true;
  };

  if (hasErrors) {return;}
  appendSoldier();
};

function clearErrors() {
  $('.alert-danger').empty();
};

function addError(errorText) {
  var errorDiv = $('.alert-danger');
  var currentError = errorDiv.html();

  if (currentError) {
    currentError += '<br>';
  };

  errorDiv.html(currentError + errorText);
};

function isValidRank(rank) {
  var ranks = ['GEN', 'LTG', 'MG', 'BG', 'COL', 'LTC', 'MAJ',
      'CPT', '1LT', '2LT', 'CW5', 'CW4', 'CW3', 'CW2', 'WO1',
      'SMA', 'CSM', 'SGM', '1SG', 'MSG', 'SFC', 'SSG', 'SGT',
      'CPL', 'SPC', 'PFC', 'PV2', 'PVT']
  normalizedRank = rank.toUpperCase();
  return ranks.indexOf(normalizedRank) != -1;
};

function appendSoldier() {
  $.get("/ajax/soldier", 
  {
    rank: rankField.val(),
    name: nameField.val(),
  }).done( 
    function(data) {
      soldiers = extractSoldiersFromList(data);
      renderHTMLListFromData(soldiers);
    }
  );

  clearInput();
};

function extractSoldiersFromList(data) {
  var soldierData = $.parseJSON(data);
  var soldiers = [];

  for (var i=0; i < soldierData.length; ++i) {
    var soldier = soldierToString(soldierData[i]);
    soldiers[i] = soldier;
  }

  return soldiers;
};

function renderHTMLListFromData(data) {
  list.empty();
  elements = data;
  addSoldiersToUL(elements);
}

function addSoldiersToUL(soldiers) {
  for (var i = 0; i < soldiers.length; ++i) {
    list.append("<li>" + soldiers[i] + "</li>");
  }
}

function clearInput() {
  rankField.val('');
  nameField.val('');
}



function soldierToString(soldierDict) {
  var rank = soldierDict.rank;
  var name = soldierDict.name;

  return rank + " " + name;
};

(Yes, I realize I am a horrible person for using GET instead of POST for this. I was having trouble getting POST to work with my back-end.)

I'm having trouble figuring out the right unit tests for the appendSoldier function. Here's my attempt:

test("appendSoldier makes request with correct args", function() {
  sinon.spy($, 'get');
  rankField.val('PVT');
  nameField.val('Snuffy');

  appendSoldier();

  ok($.get.calledWith("/ajax/soldier", {rank: 'PVT', name: 'Snuffy'}));
  $.get.restore();
});

test("UL of Soldiers populates correctly", function() {
  renderHTMLListFromData(soldierData);
  ok(list.has('li').text('PVT Snuffy'));
});

My reasoning here is that unit tests aren't supposed to test anything external, so I make sure the right request gets called, and that the proper thing is done with the response. I'm unhappy with this, because it still doesn't tell me that appendSoldier works like it's supposed to. I guess that's just something I leave to my functional tests?

I'm also not sure about my tests for the onSoldierSubmitted function, which displays appropriate errors, or calls appendSoldier if there aren't any errors. Here is my test for valid input:

test("Get request called with valid input", function() {
  sinon.spy($, "get");
  onSoldierSubmitted('pvt', 'snuffy');
  ok($.get.called, "method called");
  $.get.restore();
});

I'm not sure whether I ought to be uncomfortable that this tests that appendSoldier sends the get request, rather than testing that appendSoldier gets called.

share|improve this question
    
Welcome to Code Review! I'm not sure this question is on-topic for this site yet. Per your question, "I get the error: Attempted to wrap undefined property undefined as function" is that what you are expecting to happen, or an unexpected error? –  Phrancis Feb 16 at 19:00
    
This is unexpected. Should I just go ahead and delete that part? At this point I'm more concerned with good testing practices than figuring out why this isn't working like I want it to. –  user2201041 Feb 16 at 19:12
    
That may work, as long as the code you leave in works as expected. –  Phrancis Feb 16 at 19:17

Your Answer

 
discard

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

Browse other questions tagged or ask your own question.