Refactor non-developer codes
Based on true story
I have a chance to create a website for an internal event. Our team decides that the experience from this website could be similar to the one created from Design team — they just held similar event recently — that experience looks neat and clean.
With limited effort and time (… and laziness), I decide to use their codes as a starting point.
After working with the codes for a short period of time, I have seen patterns and practices e.g. imperative programming, dead codes, unused dependencies, that could be improved and have the idea to write them down.
Disclaimers
My purpose is not to criticize anyone but to record lessons learnt that would be beneficial for anyone at later time.
Imperative programming
Not use array method — map
This is simply to transform this.programsData to array of Session object. After scrolling, scrolling … and scrolling, it is easy to lose track of what is the outcome of this operation.
let sessions = [];
let entries = this.programsData;for (var j = 0; j < entries.length; j++) {
var newSession = new Session();
var entry = entries[j]; var ID = entry.RowKey;
if (ID) {
newSession.ID = ID;
} var title = entry.Title;
if (title) {
newSession.Title = title;
} var track = entry.Track;
if (track) {
newSession.Track = entry.Track;
} var type = entry.Type;
if (type) {
newSession.Type = type;
} var description = entry.Description;
if (description) {
newSession.Description = description;
} var language = entry.Language;
if (language) {
newSession.Language = language;
} var calendarLink = entry.CalendarLink;
if (calendarLink) {
newSession.CalendarLink = calendarLink;
} var eventX = entry.EventX;
if (eventX) {
newSession.EventX = eventX;
} var eventXText = entry.EventXText;
if (eventXText) {
newSession.EventXText = eventXText;
} else {
newSession.EventXText = "Register in EventX";
} var special = entry.Special;
if (special) {
newSession.Special = special;
} var specialText = entry.SpecialText;
if (specialText) {
newSession.SpecialText = specialText;
} var specialIcon = entry.SpecialIcon;
if (specialIcon) {
newSession.SpecialIcon = "images/em-icons.svg#" + specialIcon;
} if (special && specialText) {
if (!specialIcon) {
newSession.SpecialIcon = "images/em-icons.svg#" + "exclamation-point";
}
newSession.ShowSpecial = true;
} else {
newSession.ShowSpecial = false;
} var start = entry.Start;
if (start) {
var startDate = new Date(start);
newSession.Date = startDate;
newSession.StartTime = startDate;
} var end = entry.End;
if (end) {
var endDate = new Date(end);
newSession.EndTime = endDate;
} var speakerIDs = entry.Speakers;
if (speakerIDs) {
var speakers = speakerIDs.split(",");
var sp;
for (sp = 0; sp < speakers.length; sp++) {
if (speakers[sp].charAt(0) == " ") {
speakers[sp] = speakers[sp].substr(1);
}
}
newSession.Speakers = speakers;
} var regionIDs = entry.Regions;
if (regionIDs) {
var regions = regionIDs.split(",");
var rg;
for (rg = 0; rg < regions.length; rg++) {
if (regions[rg].charAt(0) == " ") {
regions[rg] = regions[rg].substr(1);
}
}
newSession.Regions = regions;
}
sessions.push(newSession);
}
After refactoring, the codes could be reduced from more than 100 lines to ~20 lines of codes. The data structure could be understood in a quick glance and the transforming logic is actually simple.
let sessions = this.programsData.map(program => {
return {
ID: program.RowKey,
Title: program.Title,
Track: program.Track,
Type: program.Type,
Description: program.Description,
Language: program.Language,
CalendarLink: program.CalendarLink,
EventX: program.EventX,
EventXText: program.EventXText && "Register in EventX",
Special: program.Special,
SpecialText: program.SpecialText,
SpecialIcon: program.SpecialIcon ? "images/em-icons.svg#" + program.SpecialIcon : "images/em-icons.svg#" + "exclamation-point",
ShowSpecial: program.Special && program.SpecialText,
Date: new Date(program.Start),
StartTime: new Date(program.Start),
EndTime: new Date(program.End),
Speakers: program.Speakers ? program.Speakers.split(",").map(speaker => speaker.trim()): [],
Regions: program.Regions ? program.Regions.split(",").map(region => region.trim()) : []
}
});Not use array method — find
This example is similar but shorter. This is simply to find the selected session.
getSession(ID) { for (var i = 0; i < this.sessionList.length; i++) {
if (this.sessionList[i] && this.sessionList[i].ID && this.sessionList[i].ID == ID) {
return this.sessionList[i];
}
} //No session associated with that ID
return null;
}
This could be done in one line with built-in array method.
getSession(ID) {
return this.sessionList.find(session => session.ID === ID);
}Append CSS class manually
This is simply to display or hide one section of the HTML document. Imperatively, this could be done by manually manipulating the class list but this is not obvious when looking at the HTML element.
<div class="em-c-alert em-u-is-hidden" role="alert" :id="s.ID">
...
</div>openLink(ID){
var element = document.getElementById(ID);
element.classList.remove("em-u-is-hidden");
}closeLink(ID) {
var element = document.getElementById(ID);
element.classList.add("em-u-is-hidden");
element.tabindex = -0;
}
As the application is already implemented with Vue, we can simply bind the variable to toggle the CSS class.
<div class="em-c-alert" v-bind:class="{ 'em-u-is-hidden': !s.isLinkDisplayed }" role="alert" :id="s.ID">
...
</div>openLink(session){
session.isLinkDisplayed = true;
}closeLink(session) {
session.isLinkDisplayed = false;
}
Multiple Responsibilities in a single component
There are literally 6 subpages (Home, Program, Program Detail, Team, Speaker Detail, FAQ) in one single component. Instead of breaking these into its own component and manage the routes through Vue Router, this component manage the routes itself with boolean flags. The conceptual overview is as following:
- A user tries to get access to the subpage (by clicking the link or enter the URL manually)
- Each link maps to its own URL but all are linked to the same component!!! with different props.
- Inside the component, it sets the boolean flags, i.e.,
profileView, profilesView, sessionsView, sessionView, aboutView, FAQViewaccording to the props. It later uses these flags to display/hide subpages in the component.
This results in one giant component containing 4350 lines of codes!!! This is very cumbersome.
Unused dependencies
Initially, I was surprised by the tremendous number of package dependencies. I use depcheck to analyze the dependencies so I can decide to remove. Unfortunately, there are quite number of false alerts — cases in which a dependency is being used but is reported as unused. To overcome these false alerts, I need to remove and re-start the application to verify whether it is working fine. At the end, I can remove half of the dependencies.
Dead code
Commented code should be removed.
