This article is extracted from the book JavaScript for Rails Developers and adapted for the web. Get your copy today. ✌️
Writing code is not a linear process—you can’t always see what needs to be written when staring at a blank canvas. In this article, I want to document the steps I took to write a method from from a larger codebase. You’ll see how it evolved from a highly procedural structure with difficult to follow if/else branches into a more object-oriented design that is actually followable and more straightforward to maintain.
For context: the feature of this code allows lines of a code editor (that is built in the book) to be moved up and down using the arrow keys. References like view, lineAt, dispatch and so on are specific to the underlying library, CodeMirror, that is being used.
Quick reminder: when refactoring, the behavior of the code should remain unchanged. In this case, that means I can’t alter the result of it and how the moveLine method is used in its parent context.
First look at the initial version I started with. You can see how the code closely follows my thought process, focusing on what I needed the feature to accomplish:
- If the direction (from the arrow key) is up, dispatch the changes—unless duplicate is true.
- If the direction is down, do the same, but in reverse.
export const moveLine = (view, direction, { duplicate = false } = {}) => {
const document = view.state.doc
const selection = view.state.selection.main
const line = document.lineAt(selection.from)
const lineCount = document.lines
if (direction === "up") {
if (duplicate) {
const changes = {
from: line.from,
to: line.from,
insert: line.text + "\n"
}
view.dispatch({
changes,
selection: {
anchor: selection.anchor,
head: selection.head
}
})
} else if (line.number > 1) {
const prevLine = document.lineAt(line.from - 1)
const changes = {
from: prevLine.from,
to: line.to,
insert: line.text + "\n" + prevLine.text
}
view.dispatch({
changes,
selection: {
anchor: selection.anchor - prevLine.length - 1,
head: selection.head - prevLine.length - 1
}
})
}
return true
} else if (direction === "down") {
if (duplicate) {
const changes = {
from: line.to,
to: line.to,
insert: "\n" + line.text
}
view.dispatch({
changes,
selection: {
anchor: selection.anchor + line.length + 1,
head: selection.head + line.length + 1
}
})
} else if (line.number < lineCount) {
const nextLine = document.lineAt(line.to + 1)
const changes = {
from: line.from,
to: nextLine.to,
insert: nextLine.text + "\n" + line.text
}
view.dispatch({
changes,
selection: {
anchor: selection.anchor + nextLine.length + 1,
head: selection.head + nextLine.length + 1
}
})
}
return true
}
return false
}
My first step was to extract the logic from each branch into its own method. This makes it much easier to identify patterns within the code:
- the dispatch pattern (
view.dispatch()); - the changes object (
const changes = {}); - the selection calculation (
selection.anchor/head - length - 1).
export const moveLine = (view, direction, { duplicate = false } = {}) => {
const document = view.state.doc
const selection = view.state.selection.main
const currentLine = document.lineAt(selection.from)
const totalLines = document.lines
if (direction === "up") {
return moveLineUp(view, document, selection, currentLine, duplicate)
} else if (direction === "down") {
return moveLineDown(view, document, selection, currentLine, totalLines, duplicate)
}
return false
}
const moveLineUp = (view, document, selection, currentLine, duplicate) => {
if (duplicate) {
return duplicateLineUp(view, selection, currentLine)
} else if (currentLine.number > 1) {
return shiftLineUp(view, document, selection, currentLine)
}
return false
}
const moveLineDown = (view, document, selection, currentLine, totalLines, duplicate) => {
if (duplicate) {
return duplicateLineDown(view, selection, currentLine)
} else if (currentLine.number < totalLines) {
return shiftLineDown(view, document, selection, currentLine)
}
return false
}
const duplicateLineUp = (view, selection, currentLine) => {
const changes = {
from: currentLine.from,
to: currentLine.from,
insert: `${currentLine.text}\n`
}
view.dispatch({
changes,
selection: { anchor: selection.anchor, head: selection.head }
})
return true
}
const duplicateLineDown = (view, selection, currentLine) => {
const changes = {
from: currentLine.to,
to: currentLine.to,
insert: `\n${currentLine.text}`
}
view.dispatch({
changes,
selection: {
anchor: selection.anchor + currentLine.length + 1,
head: selection.head + currentLine.length + 1
}
})
return true
}
const shiftLineUp = (view, document, selection, currentLine) => {
const previousLine = document.lineAt(currentLine.from - 1)
const changes = {
from: previousLine.from,
to: currentLine.to,
insert: `${currentLine.text}\n${previousLine.text}`
}
view.dispatch({
changes,
selection: {
anchor: selection.anchor - previousLine.length - 1,
head: selection.head - previousLine.length - 1
}
})
return true
}
const shiftLineDown = (view, document, selection, currentLine) => {
const nextLine = document.lineAt(currentLine.to + 1)
const changes = {
from: currentLine.from,
to: nextLine.to,
insert: `${nextLine.text}\n${currentLine.text}`
}
view.dispatch({
changes,
selection: {
anchor: selection.anchor + nextLine.length + 1,
head: selection.head + nextLine.length + 1
}
})
return true
}
Next, I wanted to remove the duplicated dispatch pattern and include it only once in the moveLine method. Doing this helped highlight even clearer patterns in the returned objects.
export const moveLine = (view, direction, { duplicate = false } = {}) => {
const document = view.state.doc
const selection = view.state.selection.main
const currentLine = document.lineAt(selection.from)
const totalLines = document.lines
let operation
if (direction === "up") {
operation = moveLineUp(document, selection, currentLine, duplicate)
} else if (direction === "down") {
operation = moveLineDown(document, selection, currentLine, totalLines, duplicate)
}
if (!operation) return false
view.dispatch({
changes: operation.changes,
selection: operation.selection
})
return true
}
const moveLineUp = (document, selection, currentLine, duplicate) => {
if (duplicate) {
return duplicateLineUp(selection, currentLine)
} else if (currentLine.number > 1) {
return shiftLineUp(document, selection, currentLine)
}
return null
}
const moveLineDown = (document, selection, currentLine, totalLines, duplicate) => {
if (duplicate) {
return duplicateLineDown(selection, currentLine)
} else if (currentLine.number < totalLines) {
return shiftLineDown(document, selection, currentLine)
}
return null
}
const duplicateLineUp = (selection, currentLine) => ({
changes: {
from: currentLine.from,
to: currentLine.from,
insert: `${currentLine.text}\n`
},
selection: {
anchor: selection.anchor,
head: selection.head
}
})
const duplicateLineDown = (selection, currentLine) => ({
changes: {
from: currentLine.to,
to: currentLine.to,
insert: `\n${currentLine.text}`
},
selection: {
anchor: selection.anchor + currentLine.length + 1,
head: selection.head + currentLine.length + 1
}
})
const shiftLineUp = (document, selection, currentLine) => {
const previousLine = document.lineAt(currentLine.from - 1)
return {
changes: {
from: previousLine.from,
to: currentLine.to,
insert: `${currentLine.text}\n${previousLine.text}`
},
selection: {
anchor: selection.anchor - previousLine.length - 1,
head: selection.head - previousLine.length - 1
}
}
}
const shiftLineDown = (document, selection, currentLine) => {
const nextLine = document.lineAt(currentLine.to + 1)
return {
changes: {
from: currentLine.from,
to: nextLine.to,
insert: `${nextLine.text}\n${currentLine.text}`
},
selection: {
anchor: selection.anchor + nextLine.length + 1,
head: selection.head + nextLine.length + 1
}
}
}
I wanted to try moving the duplicated method into the direction logic instead, just to see how that would work.
export const moveLine = (view, direction, { duplicate = false } = {}) => {
const document = view.state.doc
const selection = view.state.selection.main
const currentLine = document.lineAt(selection.from)
const totalLines = document.lines
let operation
if (direction === "up") {
operation = moveLineUp(document, selection, currentLine, duplicate)
} else if (direction === "down") {
operation = moveLineDown(document, selection, currentLine, totalLines, duplicate)
}
if (!operation) return false
view.dispatch({
changes: operation.changes,
selection: operation.selection
})
return true
}
const moveLineUp = (document, selection, currentLine, duplicate) => {
if (currentLine.number === 1 && !duplicate) return null
if (duplicate) {
return {
changes: {
from: currentLine.from,
to: currentLine.from,
insert: `${currentLine.text}\n`
},
selection: {
anchor: selection.anchor,
head: selection.head
}
}
}
const previousLine = document.lineAt(currentLine.from - 1)
return {
changes: {
from: previousLine.from,
to: currentLine.to,
insert: `${currentLine.text}\n${previousLine.text}`
},
selection: {
anchor: selection.anchor - previousLine.length - 1,
head: selection.head - previousLine.length - 1
}
}
}
const moveLineDown = (document, selection, currentLine, totalLines, duplicate) => {
if (currentLine.number === totalLines && !duplicate) return null
if (duplicate) {
return {
changes: {
from: currentLine.to,
to: currentLine.to,
insert: `\n${currentLine.text}`
},
selection: {
anchor: selection.anchor + currentLine.length + 1,
head: selection.head + currentLine.length + 1
}
}
}
const nextLine = document.lineAt(currentLine.to + 1)
return {
changes: {
from: currentLine.from,
to: nextLine.to,
insert: `${nextLine.text}\n${currentLine.text}`
},
selection: {
anchor: selection.anchor + nextLine.length + 1,
head: selection.head + nextLine.length + 1
}
}
}
I got stuck at this point. There’s still so much happening on every line, and it’s still not easy to follow. I want to try moving it into a class (this would mean changing one of the refactoring rules, but let’s work around it for now).
class Line {
constructor(view, direction, duplicate = false) {
this.view = view
this.direction = direction
this.duplicate = duplicate
}
move() {
if (!this.duplicate && this.#atBoundary) return false
const operation = this.duplicate
? this.#duplicate()
: this.#swap()
this.view.dispatch(operation)
return true
}
// private
#duplicate() {
return {
changes: this.#duplicateChanges,
selection: this.#newSelection
}
}
#swap() {
return {
changes: this.#swapChanges,
selection: this.#newSelection
}
}
get #document() {
return this.view.state.doc
}
get #selection() {
return this.view.state.selection.main
}
get #currentLine() {
return this.#document.lineAt(this.#selection.from)
}
get #moveUp() {
return this.direction === "up"
}
get #targetLine() {
return this.#document.lineAt(
this.#moveUp ? this.#currentLine.from - 1 : this.#currentLine.to + 1
)
}
get #atBoundary() {
return this.#moveUp
? this.#currentLine.number === 1
: this.#currentLine.number === this.#document.lines
}
get #selectionOffset() {
if (this.duplicate && this.#moveUp) return 0
const lineLength = this.duplicate ? this.#currentLine.length : this.#targetLine.length
return (lineLength + 1) _ (this.#moveUp ? -1 : 1)
}
get #duplicateChanges() {
return {
from: this.#moveUp ? this.#currentLine.from : this.#currentLine.to,
to: this.#moveUp ? this.#currentLine.from : this.#currentLine.to,
insert: this.#moveUp ? `${this.#currentLine.text}\n` : `\n${this.#currentLine.text}`
}
}
get #swapChanges() {
return {
from: this.#moveUp ? this.#targetLine.from : this.#currentLine.from,
to: this.#moveUp ? this.#currentLine.to : this.#targetLine.to,
insert: this.#moveUp
? `${this.#currentLine.text}\n${this.#targetLine.text}`
: `${this.#targetLine.text}\n${this.#currentLine.text}`
}
}
get #newSelection() {
return {
anchor: this.#selection.anchor + this.#selectionOffset,
head: this.#selection.head + this.#selectionOffset
}
}
}
export const moveLine = (view, direction, { duplicate = false } = {}) => {
const line = new Line(view, direction, duplicate)
return line.move()
}
And wow, this class is actually readable! Previously, you had to scan most of the method to understand what was happening. With this class, you only need to focus on the move method. If you’re interested in any of the internals, you can dive deeper into those private (getter) methods. Sure, the majority of the logic is still there, but now it’s encapsulated in smaller methods, making it objectively easier to understand.
This class works because of the moveLine export, which returns the result of the move method from Line. As a result, it can still be imported in its parent as moveLine.
This is a perfect example of a situation where I should have taken a step back quicker and followed my own advice: if it’s more than a few lines of code, you likely need to create a class.
Interested in making JavaScript your second-favorite language? Get your copy of JavaScript for Rails Developers today.
