3
\$\begingroup\$

I created a small Javascript elevator simulation that works as expected, however, being pretty new to Javascript I can t help but feel like this code in unnecessarily complex. Honestly any feedback would be appreciated.

const doorOpenSpeed = 3

var Elevator = function() {
	this.currentFloor = 1;
	// 1 floor ever 3 seconds
	this.speed = 3;
	this.doorOpenTime = 3;
	this.doorsOpen = false;
	this.isMoving = false;
	this.availableFloors = Array.from({length: 10}, (min, max) => max+1); 
	
	this.openDoors = function(doors) {
		for (let i = 0; i < doors.length; i++) {
			doors[i].classList.add(doors[i].id + "-moved");
		} 
		this.doorsOpen = true;

	}
	this.closeDoors = function(doors) {
		for (let i = 0; i < doors.length; i++) {
			doors[i].classList.remove(doors[i].id + "-moved");
		}
		this.doorsOpen = false;
	}
}

var Panel = function(elevator) {

	var enter = document.querySelector("#enter");
	var inputButtons = document.querySelectorAll(".panel-input-button");

	this.updateDisplay = function(floor) {
		var floorNumber = document.querySelector("#floor-number");
		floorNumber.innerHTML = floor;
	}
	this.displayInput = function(button) {
		var display = document.querySelector("#panel-display");
		display.innerHTML = button.id;
	}

	this.pauseInput = function() {
		elevator.isMoving = false;
		var arrivalNotification = document.querySelector("#arrived-notification");
		arrivalNotification.innerHTML = "*";
	}

	this.moveElevator = function() {
		var requestedFloor = document.querySelector("#panel-display").innerHTML;
		if (!elevator.isMoving) {
			if (requestedFloor in elevator.availableFloors) {
				if (requestedFloor != elevator.currentFloor) {
					elevator.isMoving = true;
					var arrivalNotification = document.querySelector("#arrived-notification");
					var upArrow = document.querySelector("#up-arrow");
					var downArrow = document.querySelector("#down-arrow");
					arrivalNotification.innerHTML = "";
					this.updateDisplay(requestedFloor);

					if (requestedFloor > elevator.currentFloor) {
						upArrow.classList.remove("hide");
						downArrow.classList.add("hide");
					}
					else {
						downArrow.classList.remove("hide");
						upArrow.classList.add("hide");
					}

					var travelTime = (Math.abs(requestedFloor - elevator.currentFloor) * 
					                         (elevator.speed * 1000)) + 
											 (doorOpenSpeed * 1000) +
					                         (elevator.doorOpenTime * 1000);
					var arrivalTime = (Math.abs(requestedFloor - elevator.currentFloor) * 
					                         (elevator.speed * 1000)) + 
											 (doorOpenSpeed * 1000) +
					                         (elevator.doorOpenTime * 2000);

					setTimeout(this.pauseInput.bind(this), travelTime);
					elevator.currentFloor = requestedFloor;
					var elevatorDoors = document.querySelectorAll(".elevator-door");
					elevator.openDoors(elevatorDoors);
					setTimeout(elevator.closeDoors.bind(this, elevatorDoors), elevator.doorOpenTime * 1000);
					setTimeout(elevator.openDoors.bind(this, elevatorDoors), arrivalTime);
					setTimeout(elevator.closeDoors.bind(this, elevatorDoors), arrivalTime + (elevator.doorOpenTime * 1000));
				}
			}
		}
	}

	enter.addEventListener("click", this.moveElevator.bind(this), true);
	for (let i = 0; i < inputButtons.length; i++) {
		inputButtons[i].addEventListener("click", this.displayInput.bind(this, inputButtons[i]), true);
	}
}

var elevator = new Elevator;
var panel = new Panel(elevator);
table {
	width: 100%;
	table-layout: fixed;
	border-spacing:7px 7px;
}

td {
	border: 1px solid black;
	border-radius: 50%;
	text-align: center;
	cursor: pointer;
}

.left-elevator-door-moved {
	transform: translate(-125px);
	transition: 3s;

}

.right-elevator-door-moved {
	transform: translate(125px);
	transition: 3s;
}

#container {
	height: 500px;
}

#feedback-panel {
	width: 100px;
	height: 50px;
	margin: 10px 10px 10px 75px;
	background-color: black;
	color: red;
	display: flex;
	align-items: center;
}
#arrived-notification {
	display: inline;
	margin-left: 13px;
	margin-top: 7px;
	font-size: 28px;
	flex: 1;
}

#directional-arrows {
	flex-direction: column;
	margin-left: 10px;
	flex: 1;
}


#floor-number {
	display: inline;
	margin-left: 13px;
	font-size: 28px;
	flex: 1;
}

#elevator {
	width: 250px;
	height: 400px;
	background-color: #4d4d4d;
	display: inline-flex;
	overflow: hidden;
}

#left-elevator-door {
	justify-content: space-between;
	padding: 0px;
	margin: 0px;
	width: 50%;
	height: 100%;
	background-color: #bcc6cc;
	border-right: 3px solid black;
	align-self: left;
	transition: 3s;

}

#right-elevator-door {
	justify-content: space-between;
	padding: 0px;
	margin: 0px;
	width: 50%;
	height: 100%;
	background-color: #bcc6cc;
	border-left: 3px solid black;
	transition: 3s;

}

#panel {
	background-color: #bcc6cc;
	border: 1px solid black;
	width: 60px;
	height: 210px;
	position: relative;
	bottom: 50%;
	left: 300px;

}

#panel-display {
	background-color: #000000;
	height: 15px;
	width: 70%;
	margin: 5px auto;
	text-align: center;
	color: red;
	padding: 5px;
}

#panel-input {
	width: 100%;
	display: block;
	margin: 0 auto;
}

#enter {
	border: 1px solid black;
	border-radius: 5px;
	text-align: center;
	margin: 1px auto;
	width: 80%;
	cursor: pointer;

}

.hide {
	display: none !important;
}
<!DOCTYPE html>
<html>
<head>
	<link rel="stylesheet" type="text/css" href="style.css">
	<title>Jelevator</title>
</head>
<body>
	<div id="container">
		<div id="feedback-panel">

			<div id="directional-arrows">
					<img id="up-arrow" src="https://drive.google.com/uc?export=view&id=1qdazZI7Up-xMaidLLorgYtTTzP_pOjAD" height="20px" width="20px">
					<img id="down-arrow" src="https://drive.google.com/uc?export=view&id=1bDPC6QDSA9__swiKJr8wLcfLr_GtUPyM" height="20px" width="20px">
			</div>
			<div id="floor-number">1</div>
			<div class="arrived" id="arrived-notification"></div>
		</div>
		
		<div id="elevator">
			<div class="elevator-door" id="left-elevator-door"></div>
			<div class="elevator-door" id="right-elevator-door"></div>

		</div>
		<div id="panel">
			<div id="panel-display"></div>
			<div id="panel-input">
				<table>
					<tr>
						<td class="panel-input-button" id="1">1</td>
						<td class="panel-input-button" id="2">2</td>
					</tr>
					<tr>
						<td class="panel-input-button" id="3">3</td>					
						<td class="panel-input-button" id="4">4</td>
					</tr>
					<tr>
						<td class="panel-input-button" id="5">5</td>
						<td class="panel-input-button" id="6">6</td>
					</tr>
					<tr>
						<td class="panel-input-button" id="7">7</td>
						<td class="panel-input-button" id="8">8</td>
					</tr>
					<tr>
						<td class="panel-input-button" id="9">9</td>
						<td class="panel-input-button" id="10">10</td>
					</tr>
				</table>
				<div id="enter">Enter</div>
			</div>
		</div>
	</div>
	
	<script src="main.js"></script>
</body>
</html>

\$\endgroup\$
2
\$\begingroup\$

I think an elevator rider would want to know the passing floors.


Using table for layout is frowned upon, for good reasons. With any real layout complexity you quickly descend to the 7th level of html hell. Do not insist on re-learning the harsh lessons of the past, just believe me when I say do not do it.


< dryHumor > The juxtaposition of good variable names with magic numbers is an excellent illustration of the "don't use magic numbers" trope. Good job < / dryHumor >


The unnecessary complexity comes from mundging elevator display code with elevator functionality. The elevator object should have zero display code. The elevator should be able to run with zero regard for its display. Instead the display could subscribe to a change event, let's say, for the current floor updates. In other words you want an Elevator API.

The present simulator is very resistant to change. For example think about the extensive existing code changes to handle stops as riders press buttons on intermediate floors while the elevator is moving. This will be far easier and far less buggy given a Single Responsibility compliant design. A less ambitious project i.e. Small ... Simulation, is flawed rationale for disregarding general OO design and coding.

Look at the real parts of a problem domain as the starting guide for objects (classes). Think about what these things do - functionality. IMO this is a better guide for where state should reside. If you're writting shorter, less complex class/function code but more of them then you're going in the right direction. Good design manages overall complexity by distributing it into appropriate, focused (read 'single responsibility') classes and functions.

\$\endgroup\$
1
\$\begingroup\$

Feedback

The simulator itself looks nice. I like the simple layout of the number pad, as well as the animations on the door. The colors look nice as well. As far as the code goes, there are some suggestions outlined below. I do like the use of partially applied functions in the calls to setTimeout() as well as the use of addEventListener() for the UI events.

Suggestions

Many of the suggestions below come from experience as well as articles like this one.

Prototype methods

With idiomatic javascript, methods are typically defined on the prototype instead of in the constructor function. So instead of

this.openDoors = function(doors) {

that would be added outside the constructor, on the prototype:

Elevator.prototype.openDoors = function(doors) {

Read more about this topic on the MDN documentation for Modifying Prototypes

Ecmascript 2015 features

It appears that some features are used, like const:

const doorOpenSpeed = 3

And arrow functions:

this.availableFloors = Array.from({length: 10}, (min, max) => max+1); 

In that case, classes could be used for Elevator and Panel.

Selecting elements by Id

There are many calls to select elements using the id attribute with document.querySelector(). For example:

var enter = document.querySelector("#enter"); 

While this works, it would be much faster to use document.getElementById().

var enter = document.getElementById("enter"); 

For a comparison, see this performance test.

Cache DOM references

It may not make a large difference on a small page like this but if you work on a larger application with many more elements to be accessed via the DOM, it is wise to store them in a variable (preferably using const to avoid re-assignment) and use that variable whenever necessary.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.