More oof-worthy Marketo Forms 2.0 code in the wild

Saying bad code — including my own old code! — causes physical pain isn’t a figure of speech. My jaw locks, stomach clenches, teeth grind.

I shake it off quickly. But the stuff people fearlessly publish on enterprise websites continues to amaze.

This forms JS, intended to localize label text, was seen in the wild:

MktoForms2.whenReady(function (mktoForm) {
   var lang = "";
   console.log(lang.languageForm);

   lang = mktoForm.getValues();
   console.log(lang.languageForm);
   if (lang.languageForm == "de") {
      var Request = document.getElementById("Lblrequestinvestors");
      Request.innerText = "Bestellung";
      var Version = document.getElementById("Lbllanguage");
      Version.innerHTML = "Sprache";
      var Firstname = document.getElementById("LblFirstName");
      Firstname.innerHTML = "Vorname";
      var Lastname = document.getElementById("LblLastName");
      Lastname.innerHTML = "Nachname";
      var Email = document.getElementById("LblEmail");
      Email.innerHTML = "Email";
      var Phone = document.getElementById("LblPhone");
      Phone.innerHTML = "Telefon";
      var Companyname = document.getElementById("LblCompany");
      Companyname.innerHTML = "Firma";
      var Address = document.getElementById("LblAddress");
      Address.innerHTML = "Adresse";
      var City = document.getElementById("LblCity");
      City.innerHTML = "Stadt";
      var Country = document.getElementById("LblCountry");
      Country.innerHTML = "Land";
      var Zip = document.getElementById("LblPostalCode");
      Zip.innerHTML = "PLZ";
      var Comment = document.getElementById("LblinquiryMessage");
      Comment.innerHTML = "Kommentar";
   }
});

Let’s break this down.

It violates the DRY principle by including document.getElementById and Element.innerHTML repeatedly instead of looping over a collection.

It violates general KISS principles by declaring a bunch of unnecessary variables instead of setting properties on an object.

It uses incorrect variable naming because mktoForm.getValues() returns all field names and values, not just the lang(uage).

It draws focus to the wrong data by implying the <label id value is important, while it’s the <label for that really determines the related input.

And, leaving all the above aside, it also has a pretty significant bug. I deliberately won’t spoil the challenge by fixing it in my better code below. Can you see what bug remains? Let me know in the comments.

Easing the pain

Here’s a more readable and extensible way to accomplish the same thing:

MktoForms2.whenReady(function (mktoForm) {
   
   let formEl = mktoForm.getFormElem()[0],
       currentValues = mktoForm.getValues(),
       currentFormLanguage = currentValues.languageForm;

   let translations = {
      de: {
         requestinvestors: "Bestellung",
         language: "Sprache",
         FirstName: "Vorname",
         LastName: "Nachname",
         Email: "Email",
         Phone: "Telefon",
         Company: "Firma",
         Address: "Adresse",
         City: "Stadt",
         Country: "Land",
         PostalCode: "PLZ",
         inquiryMessage: "Kommentar"
      }
   };   
   
   let currentTranslateMap = translations[currentFormLanguage];
   
   if(currentTranslateMap) {
      Object.keys(currentTranslateMap)
      .forEach(function(fieldName){
         let fieldLabel = formEl.querySelector("label[for='" + fieldName + "']");
         
         if(fieldLabel) {
           fieldLabel.innerHTML = currentTranslateMap[fieldName];
         }
      });
   }
   
});

You’ll note the better code isn’t necessarily shorter to start. Better code isn’t about brevity: 1 line of opaque and fragile code isn’t better than 3 lines of self-documenting and resilient code.

But even if you do care about raw LOC, extending my code takes fewer lines. Adding any new field to the original code takes 2 lines, while adding a new field to my code takes 1 line (another key-value in translations.de) and no copy-and-pasted function calls.