Singly Linked List Implementation in ES6
$begingroup$
This is my Singly Linked List implementation in ES6.
class LinkedList{
constructor(){
this.head = null;
this.size = 0;
}
insert(item){
this.size ++;
let p = new Node(item);
if(this.head === null){
this.head = p;
}
else{
p.next = this.head;
this.head = p;
}
}
insertAt(item, index){
if(index > this.size){
throw "Wrong index";
}
let p = new Node(item);
if(index === 0){
p.next = this.head;
this.head = p;
return;
}
let curr = this.head;
while(curr !== null){
if(index === 0){
if(curr.next === null){
curr.next = p;
return;
}
else if(curr.next.next === null){
curr.next = p;
}else{
curr.next.next = p.next;
curr.next = p;
}
return;
}
curr = curr.next;
index --;
}
}
remove(item){
let curr, prev;
prev = curr = this.head;
if(item === curr.val){
this.head = this.head.next;
this.size --;
return;
}
while(curr !== null){
if (curr.val === item){
prev.next = curr.next;
this.size --;
return;
}
prev = curr;
curr = curr.next;
}
throw "Item doesn't exist in list."
}
length(){
return this.size;
}
is_empty(){
return this.size === 0;
}
printList(){
let curr = this.head;
let out = ;
while(curr !== null){
if(curr.next === null){
out.push(curr.val);
}else{
out.push(curr.val);
out.push("->")
}
curr = curr.next;
}
return out.join("");
}
}
class Node{
constructor(val){
this.val = val;
this.next = null;
}
}
const list = new LinkedList();
list.insert(12);
list.insert(9);
list.insert(13);
list.insert(17);
console.log(list.printList());
list.remove(12);
console.log(list.printList());
console.log(list.length());
console.log(list.is_empty());
list.insertAt(21, 2);
console.log(list.printList());
Invite reviews please on all fronts, improvements, modern usage of JS.
Thanks.
javascript linked-list ecmascript-6
$endgroup$
add a comment |
$begingroup$
This is my Singly Linked List implementation in ES6.
class LinkedList{
constructor(){
this.head = null;
this.size = 0;
}
insert(item){
this.size ++;
let p = new Node(item);
if(this.head === null){
this.head = p;
}
else{
p.next = this.head;
this.head = p;
}
}
insertAt(item, index){
if(index > this.size){
throw "Wrong index";
}
let p = new Node(item);
if(index === 0){
p.next = this.head;
this.head = p;
return;
}
let curr = this.head;
while(curr !== null){
if(index === 0){
if(curr.next === null){
curr.next = p;
return;
}
else if(curr.next.next === null){
curr.next = p;
}else{
curr.next.next = p.next;
curr.next = p;
}
return;
}
curr = curr.next;
index --;
}
}
remove(item){
let curr, prev;
prev = curr = this.head;
if(item === curr.val){
this.head = this.head.next;
this.size --;
return;
}
while(curr !== null){
if (curr.val === item){
prev.next = curr.next;
this.size --;
return;
}
prev = curr;
curr = curr.next;
}
throw "Item doesn't exist in list."
}
length(){
return this.size;
}
is_empty(){
return this.size === 0;
}
printList(){
let curr = this.head;
let out = ;
while(curr !== null){
if(curr.next === null){
out.push(curr.val);
}else{
out.push(curr.val);
out.push("->")
}
curr = curr.next;
}
return out.join("");
}
}
class Node{
constructor(val){
this.val = val;
this.next = null;
}
}
const list = new LinkedList();
list.insert(12);
list.insert(9);
list.insert(13);
list.insert(17);
console.log(list.printList());
list.remove(12);
console.log(list.printList());
console.log(list.length());
console.log(list.is_empty());
list.insertAt(21, 2);
console.log(list.printList());
Invite reviews please on all fronts, improvements, modern usage of JS.
Thanks.
javascript linked-list ecmascript-6
$endgroup$
add a comment |
$begingroup$
This is my Singly Linked List implementation in ES6.
class LinkedList{
constructor(){
this.head = null;
this.size = 0;
}
insert(item){
this.size ++;
let p = new Node(item);
if(this.head === null){
this.head = p;
}
else{
p.next = this.head;
this.head = p;
}
}
insertAt(item, index){
if(index > this.size){
throw "Wrong index";
}
let p = new Node(item);
if(index === 0){
p.next = this.head;
this.head = p;
return;
}
let curr = this.head;
while(curr !== null){
if(index === 0){
if(curr.next === null){
curr.next = p;
return;
}
else if(curr.next.next === null){
curr.next = p;
}else{
curr.next.next = p.next;
curr.next = p;
}
return;
}
curr = curr.next;
index --;
}
}
remove(item){
let curr, prev;
prev = curr = this.head;
if(item === curr.val){
this.head = this.head.next;
this.size --;
return;
}
while(curr !== null){
if (curr.val === item){
prev.next = curr.next;
this.size --;
return;
}
prev = curr;
curr = curr.next;
}
throw "Item doesn't exist in list."
}
length(){
return this.size;
}
is_empty(){
return this.size === 0;
}
printList(){
let curr = this.head;
let out = ;
while(curr !== null){
if(curr.next === null){
out.push(curr.val);
}else{
out.push(curr.val);
out.push("->")
}
curr = curr.next;
}
return out.join("");
}
}
class Node{
constructor(val){
this.val = val;
this.next = null;
}
}
const list = new LinkedList();
list.insert(12);
list.insert(9);
list.insert(13);
list.insert(17);
console.log(list.printList());
list.remove(12);
console.log(list.printList());
console.log(list.length());
console.log(list.is_empty());
list.insertAt(21, 2);
console.log(list.printList());
Invite reviews please on all fronts, improvements, modern usage of JS.
Thanks.
javascript linked-list ecmascript-6
$endgroup$
This is my Singly Linked List implementation in ES6.
class LinkedList{
constructor(){
this.head = null;
this.size = 0;
}
insert(item){
this.size ++;
let p = new Node(item);
if(this.head === null){
this.head = p;
}
else{
p.next = this.head;
this.head = p;
}
}
insertAt(item, index){
if(index > this.size){
throw "Wrong index";
}
let p = new Node(item);
if(index === 0){
p.next = this.head;
this.head = p;
return;
}
let curr = this.head;
while(curr !== null){
if(index === 0){
if(curr.next === null){
curr.next = p;
return;
}
else if(curr.next.next === null){
curr.next = p;
}else{
curr.next.next = p.next;
curr.next = p;
}
return;
}
curr = curr.next;
index --;
}
}
remove(item){
let curr, prev;
prev = curr = this.head;
if(item === curr.val){
this.head = this.head.next;
this.size --;
return;
}
while(curr !== null){
if (curr.val === item){
prev.next = curr.next;
this.size --;
return;
}
prev = curr;
curr = curr.next;
}
throw "Item doesn't exist in list."
}
length(){
return this.size;
}
is_empty(){
return this.size === 0;
}
printList(){
let curr = this.head;
let out = ;
while(curr !== null){
if(curr.next === null){
out.push(curr.val);
}else{
out.push(curr.val);
out.push("->")
}
curr = curr.next;
}
return out.join("");
}
}
class Node{
constructor(val){
this.val = val;
this.next = null;
}
}
const list = new LinkedList();
list.insert(12);
list.insert(9);
list.insert(13);
list.insert(17);
console.log(list.printList());
list.remove(12);
console.log(list.printList());
console.log(list.length());
console.log(list.is_empty());
list.insertAt(21, 2);
console.log(list.printList());
Invite reviews please on all fronts, improvements, modern usage of JS.
Thanks.
class LinkedList{
constructor(){
this.head = null;
this.size = 0;
}
insert(item){
this.size ++;
let p = new Node(item);
if(this.head === null){
this.head = p;
}
else{
p.next = this.head;
this.head = p;
}
}
insertAt(item, index){
if(index > this.size){
throw "Wrong index";
}
let p = new Node(item);
if(index === 0){
p.next = this.head;
this.head = p;
return;
}
let curr = this.head;
while(curr !== null){
if(index === 0){
if(curr.next === null){
curr.next = p;
return;
}
else if(curr.next.next === null){
curr.next = p;
}else{
curr.next.next = p.next;
curr.next = p;
}
return;
}
curr = curr.next;
index --;
}
}
remove(item){
let curr, prev;
prev = curr = this.head;
if(item === curr.val){
this.head = this.head.next;
this.size --;
return;
}
while(curr !== null){
if (curr.val === item){
prev.next = curr.next;
this.size --;
return;
}
prev = curr;
curr = curr.next;
}
throw "Item doesn't exist in list."
}
length(){
return this.size;
}
is_empty(){
return this.size === 0;
}
printList(){
let curr = this.head;
let out = ;
while(curr !== null){
if(curr.next === null){
out.push(curr.val);
}else{
out.push(curr.val);
out.push("->")
}
curr = curr.next;
}
return out.join("");
}
}
class Node{
constructor(val){
this.val = val;
this.next = null;
}
}
const list = new LinkedList();
list.insert(12);
list.insert(9);
list.insert(13);
list.insert(17);
console.log(list.printList());
list.remove(12);
console.log(list.printList());
console.log(list.length());
console.log(list.is_empty());
list.insertAt(21, 2);
console.log(list.printList());
class LinkedList{
constructor(){
this.head = null;
this.size = 0;
}
insert(item){
this.size ++;
let p = new Node(item);
if(this.head === null){
this.head = p;
}
else{
p.next = this.head;
this.head = p;
}
}
insertAt(item, index){
if(index > this.size){
throw "Wrong index";
}
let p = new Node(item);
if(index === 0){
p.next = this.head;
this.head = p;
return;
}
let curr = this.head;
while(curr !== null){
if(index === 0){
if(curr.next === null){
curr.next = p;
return;
}
else if(curr.next.next === null){
curr.next = p;
}else{
curr.next.next = p.next;
curr.next = p;
}
return;
}
curr = curr.next;
index --;
}
}
remove(item){
let curr, prev;
prev = curr = this.head;
if(item === curr.val){
this.head = this.head.next;
this.size --;
return;
}
while(curr !== null){
if (curr.val === item){
prev.next = curr.next;
this.size --;
return;
}
prev = curr;
curr = curr.next;
}
throw "Item doesn't exist in list."
}
length(){
return this.size;
}
is_empty(){
return this.size === 0;
}
printList(){
let curr = this.head;
let out = ;
while(curr !== null){
if(curr.next === null){
out.push(curr.val);
}else{
out.push(curr.val);
out.push("->")
}
curr = curr.next;
}
return out.join("");
}
}
class Node{
constructor(val){
this.val = val;
this.next = null;
}
}
const list = new LinkedList();
list.insert(12);
list.insert(9);
list.insert(13);
list.insert(17);
console.log(list.printList());
list.remove(12);
console.log(list.printList());
console.log(list.length());
console.log(list.is_empty());
list.insertAt(21, 2);
console.log(list.printList());
javascript linked-list ecmascript-6
javascript linked-list ecmascript-6
edited Jan 12 at 21:29
ggorlen
3738
3738
asked Jan 12 at 17:22
Melissa StewartMelissa Stewart
21626
21626
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$begingroup$
TL;DR
- Style could better adhere to established standards.
insertAt
has bugs to resolve.- This class is missing basic add/remove functions which prevent it from being useful.
- Writing a linked list class in JS only makes sense from an educational standpoint; primitive arrays are efficient, universal and offer more functionality with less code.
Style
Spacing
Use a space between
)
s,{
s and keywords, e.g.
if(this.head === null){
is cleaner as
if (this.head === null) {
Add vertical spacing between lines to group logical code blocks together and separate loop, conditional and function logic from declarations. For example,
let curr = this.head;
while(curr !== null){
is easier to read as
let curr = this.head;
while (curr !== null) {
this.size ++
is clearer asthis.size++
.
Be consistent--sometimes you use
}else{
, and sometimes you use
}
else{
Your whitespace fixes can be done by putting the code into SE's Code Snippet and pressing "Tidy", then adding blank lines around blocks by hand. Generally, I don't see reason to deviate from this prescribed style.
Variable and function naming
- Function names switch between
snake_case
andcamelCase
. All JS naming should becamelCase
, except class names, which areUpperCamelCase
(as you use). - Avoid single-letter variable names like
p
unless the meaning is obvious. In this case, something likenewNode
might be clearer.
Functionality
Errors
throw "Wrong index";
is an unclear error message. What exactly is wrong with the index? Considerthrow "Index out of list bounds";
which more accurately describes the problem.
You may wish to reconsider using errors at all. I find throwing errors in JS generally less appropriate than return values because the calling code can stick to normal conditionals to handle control flow.
Also, not throwing errors is in keeping with JS's builtin library functions, which generally don't complain about invalid or missing input. For
insertAt
, for example, you could returntrue
if the insertion was successful andfalse
otherwise. If the user provides something silly as an index that causes a crash, they'll get an appropriate stack trace that likely explains the problem better than a hand-written error string.
If you do wish to stick with error throwing, ensure it is comprehensive. For example,
if (index > this.size)
doesn't handle the case whenindex < 0
which could result in difficult-to-track-down bugs for the client who has to make design assumptions based on your throw.
Then, once you've covered that scenario, it begs the question whether you should now validate that the provided input is an integer number and throw an error message for that as well.
The point is, adding errors gives the client the illusion of a comprehensive and robust set of argument and function state restrictions, which is problematic if they aren't actually robust. Throwing no errors, assuming the client is behaving, and reporting
true
/false
as to whether some function failed or not seems preferential.
Variable keywords
- Use
const
when appropriate in place oflet
. This should apply to almost everything except for loop counters, accumulator variables and runner nodes. For example,let out = ;
should beconst out = ;
. Even if the risk of bugs caused by accidental reassignment is low, this has semantic benefits.
Misleading function names
printList()
is a misleading name; it stringifies the list rather than printing. I recommend overridingtoString()
here.
insert()
usually refers to insertion at some specific element, which is the behavior yourinsertAt()
function offers. Withinsert()
, It's not obvious where the insertion is happening; one ofaddFront
,unshift
orpush
seem clearer, depending on which end of your list you decide the front to be.
Internal logic
insertAt
is not working correctly, placing items incorrectly, not at all, and neglecting to increment the size.- Consider adjusting your
insertAt
code to avoid usingreturn
s andcurr.next.next
, which is difficult to reason about and causing bugs. - In
printList
, pains are taken to conditionally add the->
arrow only for non-last elements when you can simply walk the list and useout.join("->")
. - Since your internal code relies only on
Node
objects, you can make your code cleaner by testingwhile (curr)
instead ofwhile (curr === null)
. This is debatable, because it restricts your internal logic from distinguishing betweennull
andundefined
or other falsey values, but if you trust yourself to be consistent about it, I prefer the cleaner look.
Interface
As written, I find your provided function interface insufficient for typical linked list needs. It's not obvious what functionality this class offers over, say, a primitive array.
Consider
remove(item)
. This sort of function that takes an element and searches the structure for it is best used for hashes with random access. The whole point of linked lists is to offer fast insertion and removal of front and back elements, regardless of what those elements might be. Anything in the middle is of no concern, and libraries that offer access to these elements, such as Java's LinkedList collection are generally considered to be flawed because clients may make false assumptions about the time complexity of provided operations.
Without constant time
front
andback
adds, removals and peeks, I can't foresee a reason to use this class instead of a primitive array.insert
is the only useful linked list function your interface offers (disregardingisEmpty
,printList
, etc as useful but trivial).
Rewrites
Revision #1 (same functionality)
class LinkedList {
constructor() {
this.head;
this.size = 0;
}
addFront(value) {
const newNode = new Node(value);
if (this.head) {
newNode.next = this.head;
}
this.head = newNode;
this.size++;
}
insertAt(value, idx) {
let curr = this.head;
let prev;
while (curr && idx > 0) {
prev = curr;
curr = curr.next;
idx--;
}
if (prev) {
prev.next = new Node(value);
prev.next.next = curr;
this.size++;
}
else {
this.addFront(value);
}
}
remove(value) {
let curr = this.head;
let prev;
while (curr) {
if (curr.val === value) {
if (prev) {
prev.next = curr.next;
}
else {
this.head = undefined;
}
this.size--;
return true;
}
prev = curr;
curr = curr.next;
}
return false;
}
length() {
return this.size;
}
empty() {
return !this.size;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());
Revision #2 (added front/back operations)
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());
Note that there is no popBack()
because this operation would be linear without doubly linked nodes. However, the class is sufficient to support both stacks and queues with all operations in constant time. Without the tail
pointer, only a stack could be supported. Adding popBack()
and double links would give you a deque class.
Revision #3 (supported a queue)
class Queue {
constructor() {
this.list = new LinkedList();
}
offer(value) {
this.list.addBack(value);
}
poll() {
return this.list.popFront();
}
peek() {
return this.list.peekFront();
}
empty() {
return this.list.empty();
}
toString() {
return this.list.toString();
}
}
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());
for (let i = 0; i < 5; i++) {
q.offer(i);
}
while (!q.empty()) {
console.log(q.poll());
}
After all that work, a benchmark shows that it's no faster than a primitive array. This is likely due to overhead from object creation, function calls and garbage collection, which counteracts the shifting necessary on the primitive array.
$endgroup$
1
$begingroup$
A delight to read! Dear OP, Look into iterators. That would enableforEach
coding. Also Blindman76 warning of lack of private variables inclass
is notable. You really want to protect clients from themselves and make sure the linkedList can be used only as intended.
$endgroup$
– radarbob
Jan 14 at 0:29
add a comment |
$begingroup$
Unsafe and unusable.
Class syntax is dangerous
The class syntax encourages you to expose the objects state, this means it can mutate outside your control.
New developments allow class to have private properties. #privateNamed
the hash means private. However its implementations is an abomination to the languages, we now have a situation where access type is embedded in the variable name. Names should be independent of any language constructs. Anyways I digress...
As it stands your object (class
) is unsafe, you expose head
and size
(in LinkedList
) and next
(in Node
) meaning that outside code can deliberately or by accident mutate the objects state such that your function becomes inoperable.
It is possible for your code to indefinitely block the page, meaning that the only way to fix the problem is for the page to crash or it to be forced to close.
Mutation examples
const list = new LinkedList()
list.insert("😧");
list.head.next = list.head; // cyclic link.
list.remove("☠"); // Untrappable page blocking error
list.printList(); // will crash the page with out of memory error
const list = new LinkedList()
list.size = "🙈🙉🙊";
console.log(list.length); // >> "🙈🙉🙊" nonsense
const list = new LinkedList()
list.insert("A");
const a = list.head;
list.insert("B");
const b = list.head;
list.insert("C");
list.insert("D");
const d = list.head;
b.next = d; // removes c
console.log(list.length()); // >> 4 Incorrect
var node = a;
for(let i = 0; i < list.length; i++){
console.log(node.value); // WTF throws error on 4th iteration
node = node.next;
}
Normal use errors
const list = new LinkedList()
list.insert("A");
list.insertAt("B", -1); // does not insert returning undefined.
const list = new LinkedList()
list.size = 100;
console.log(list.is__Empty()); // >> false, wrong answer the list is empty
I could go on, there are many dangers when you expose internal state. Programmers, you included will be tempted to use a shortcuts, or accidently mutate the list with catastrophic consequences.
I would never allow such a dangerous object into a project, it is unusable because of its dangerous behaviour.
Object factories
Consider using a factory to create your Object.
Factories let you create a private state via closure that can not be mutate. You can confidently use the state because it is hidden and immutable.
Factory example.
The factory returns a frozen object with the state hidden via closure. I add to functions. Iterator LinkedList.values
to iterate the list from 0
to size-1
eg console.log([...list.values()])
will list items as an array. Also linkedList.itemAt(index)
as the linked list is useless without them.
You do not get access to nodes, only the values they contain. And printList
is called toString
Note code is untested and as an example only. May contain typos or logic errors.
function LinkedList() {
var head, size = 0;
const add = (value, next) => {
size ++;
return {value, next};
}
const vetIndex = idx => isNaN(idx) || idx > size || idx < 0;
const list = Object.freeze({
get length() { return size },
get isEmpty() { return size === 0 },
insert(item) { head = add(item, head) },
insertAt(item, idx = 0) {
if (vetIndex(idx)) { throw new RangeError("Bad index") }
if (idx === size) { head = add(item, head) }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
curr.next = add(item, curr.next);
}
},
remove(item) {
if (head.value === item) {
head = head.next;
size --;
} else {
let curr = head;
while (curr && curr.next && curr.next.value !== item) { curr = curr.next }
if (curr) {
curr.next = curr.next ? curr.next.next : undefined;
size --;
}
}
},
toString() {
var str = "";
if (size) {
str += head.value;
let curr = head.next;
while (curr) {
str += "->" + curr.value;
curr = curr.next;
}
}
return str;
},
*values() {
var idx = 0;
while (idx < size) { yield list.itemAt(idx++) }
},
itemAt(idx) {
if (size && !vetIndex(idx)) {
vetIndex(idx);
if (idx === size) { return head.value }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
return curr.value
}
}
},
});
return list;
}
Some more points on your code
- Function scope variables should be declared as
var
. Show you understand the language and use the correct declaration type. - Use
const
for constants. Eg inprintList
you define an arraylet out = ;
out
is a reference to an array, the reference never changes, only the content of the array does, so use a constantconst out = ;
- Only throw if not doing so will damage state such that continuing will create undefined behaviours. You threw if an item could not be found in
remove
There is no reason to throw as it does not damage your state. Returnundefined
and let the calling function deal with their problems. - When you throw do not throw strings (many catches assume an object and rethrow if its just a string). Use appropriate error objects. eg you throw a string
throw "Wrong index";
you should throw an errorthrow new RangeError("Bad index");
Orthrow new Error("Bad index");
- Don't add redundant code. the function
insertAt
has 3 returns yet can be written with on extraelse
and abreak
, with noreturn
tokens. - Don't use
null
, itsundefined
if not defined. - Always use the shorts form. Eg
if(this.head === null)
useif(!this.head)
andif (!curr.next) { curr.next = p;} else if (!curr.next.next) { curr.next = p;}
becomesif(!curr.next || !curr.next..next) { curr.next = p }
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211380%2fsingly-linked-list-implementation-in-es6%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
TL;DR
- Style could better adhere to established standards.
insertAt
has bugs to resolve.- This class is missing basic add/remove functions which prevent it from being useful.
- Writing a linked list class in JS only makes sense from an educational standpoint; primitive arrays are efficient, universal and offer more functionality with less code.
Style
Spacing
Use a space between
)
s,{
s and keywords, e.g.
if(this.head === null){
is cleaner as
if (this.head === null) {
Add vertical spacing between lines to group logical code blocks together and separate loop, conditional and function logic from declarations. For example,
let curr = this.head;
while(curr !== null){
is easier to read as
let curr = this.head;
while (curr !== null) {
this.size ++
is clearer asthis.size++
.
Be consistent--sometimes you use
}else{
, and sometimes you use
}
else{
Your whitespace fixes can be done by putting the code into SE's Code Snippet and pressing "Tidy", then adding blank lines around blocks by hand. Generally, I don't see reason to deviate from this prescribed style.
Variable and function naming
- Function names switch between
snake_case
andcamelCase
. All JS naming should becamelCase
, except class names, which areUpperCamelCase
(as you use). - Avoid single-letter variable names like
p
unless the meaning is obvious. In this case, something likenewNode
might be clearer.
Functionality
Errors
throw "Wrong index";
is an unclear error message. What exactly is wrong with the index? Considerthrow "Index out of list bounds";
which more accurately describes the problem.
You may wish to reconsider using errors at all. I find throwing errors in JS generally less appropriate than return values because the calling code can stick to normal conditionals to handle control flow.
Also, not throwing errors is in keeping with JS's builtin library functions, which generally don't complain about invalid or missing input. For
insertAt
, for example, you could returntrue
if the insertion was successful andfalse
otherwise. If the user provides something silly as an index that causes a crash, they'll get an appropriate stack trace that likely explains the problem better than a hand-written error string.
If you do wish to stick with error throwing, ensure it is comprehensive. For example,
if (index > this.size)
doesn't handle the case whenindex < 0
which could result in difficult-to-track-down bugs for the client who has to make design assumptions based on your throw.
Then, once you've covered that scenario, it begs the question whether you should now validate that the provided input is an integer number and throw an error message for that as well.
The point is, adding errors gives the client the illusion of a comprehensive and robust set of argument and function state restrictions, which is problematic if they aren't actually robust. Throwing no errors, assuming the client is behaving, and reporting
true
/false
as to whether some function failed or not seems preferential.
Variable keywords
- Use
const
when appropriate in place oflet
. This should apply to almost everything except for loop counters, accumulator variables and runner nodes. For example,let out = ;
should beconst out = ;
. Even if the risk of bugs caused by accidental reassignment is low, this has semantic benefits.
Misleading function names
printList()
is a misleading name; it stringifies the list rather than printing. I recommend overridingtoString()
here.
insert()
usually refers to insertion at some specific element, which is the behavior yourinsertAt()
function offers. Withinsert()
, It's not obvious where the insertion is happening; one ofaddFront
,unshift
orpush
seem clearer, depending on which end of your list you decide the front to be.
Internal logic
insertAt
is not working correctly, placing items incorrectly, not at all, and neglecting to increment the size.- Consider adjusting your
insertAt
code to avoid usingreturn
s andcurr.next.next
, which is difficult to reason about and causing bugs. - In
printList
, pains are taken to conditionally add the->
arrow only for non-last elements when you can simply walk the list and useout.join("->")
. - Since your internal code relies only on
Node
objects, you can make your code cleaner by testingwhile (curr)
instead ofwhile (curr === null)
. This is debatable, because it restricts your internal logic from distinguishing betweennull
andundefined
or other falsey values, but if you trust yourself to be consistent about it, I prefer the cleaner look.
Interface
As written, I find your provided function interface insufficient for typical linked list needs. It's not obvious what functionality this class offers over, say, a primitive array.
Consider
remove(item)
. This sort of function that takes an element and searches the structure for it is best used for hashes with random access. The whole point of linked lists is to offer fast insertion and removal of front and back elements, regardless of what those elements might be. Anything in the middle is of no concern, and libraries that offer access to these elements, such as Java's LinkedList collection are generally considered to be flawed because clients may make false assumptions about the time complexity of provided operations.
Without constant time
front
andback
adds, removals and peeks, I can't foresee a reason to use this class instead of a primitive array.insert
is the only useful linked list function your interface offers (disregardingisEmpty
,printList
, etc as useful but trivial).
Rewrites
Revision #1 (same functionality)
class LinkedList {
constructor() {
this.head;
this.size = 0;
}
addFront(value) {
const newNode = new Node(value);
if (this.head) {
newNode.next = this.head;
}
this.head = newNode;
this.size++;
}
insertAt(value, idx) {
let curr = this.head;
let prev;
while (curr && idx > 0) {
prev = curr;
curr = curr.next;
idx--;
}
if (prev) {
prev.next = new Node(value);
prev.next.next = curr;
this.size++;
}
else {
this.addFront(value);
}
}
remove(value) {
let curr = this.head;
let prev;
while (curr) {
if (curr.val === value) {
if (prev) {
prev.next = curr.next;
}
else {
this.head = undefined;
}
this.size--;
return true;
}
prev = curr;
curr = curr.next;
}
return false;
}
length() {
return this.size;
}
empty() {
return !this.size;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());
Revision #2 (added front/back operations)
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());
Note that there is no popBack()
because this operation would be linear without doubly linked nodes. However, the class is sufficient to support both stacks and queues with all operations in constant time. Without the tail
pointer, only a stack could be supported. Adding popBack()
and double links would give you a deque class.
Revision #3 (supported a queue)
class Queue {
constructor() {
this.list = new LinkedList();
}
offer(value) {
this.list.addBack(value);
}
poll() {
return this.list.popFront();
}
peek() {
return this.list.peekFront();
}
empty() {
return this.list.empty();
}
toString() {
return this.list.toString();
}
}
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());
for (let i = 0; i < 5; i++) {
q.offer(i);
}
while (!q.empty()) {
console.log(q.poll());
}
After all that work, a benchmark shows that it's no faster than a primitive array. This is likely due to overhead from object creation, function calls and garbage collection, which counteracts the shifting necessary on the primitive array.
$endgroup$
1
$begingroup$
A delight to read! Dear OP, Look into iterators. That would enableforEach
coding. Also Blindman76 warning of lack of private variables inclass
is notable. You really want to protect clients from themselves and make sure the linkedList can be used only as intended.
$endgroup$
– radarbob
Jan 14 at 0:29
add a comment |
$begingroup$
TL;DR
- Style could better adhere to established standards.
insertAt
has bugs to resolve.- This class is missing basic add/remove functions which prevent it from being useful.
- Writing a linked list class in JS only makes sense from an educational standpoint; primitive arrays are efficient, universal and offer more functionality with less code.
Style
Spacing
Use a space between
)
s,{
s and keywords, e.g.
if(this.head === null){
is cleaner as
if (this.head === null) {
Add vertical spacing between lines to group logical code blocks together and separate loop, conditional and function logic from declarations. For example,
let curr = this.head;
while(curr !== null){
is easier to read as
let curr = this.head;
while (curr !== null) {
this.size ++
is clearer asthis.size++
.
Be consistent--sometimes you use
}else{
, and sometimes you use
}
else{
Your whitespace fixes can be done by putting the code into SE's Code Snippet and pressing "Tidy", then adding blank lines around blocks by hand. Generally, I don't see reason to deviate from this prescribed style.
Variable and function naming
- Function names switch between
snake_case
andcamelCase
. All JS naming should becamelCase
, except class names, which areUpperCamelCase
(as you use). - Avoid single-letter variable names like
p
unless the meaning is obvious. In this case, something likenewNode
might be clearer.
Functionality
Errors
throw "Wrong index";
is an unclear error message. What exactly is wrong with the index? Considerthrow "Index out of list bounds";
which more accurately describes the problem.
You may wish to reconsider using errors at all. I find throwing errors in JS generally less appropriate than return values because the calling code can stick to normal conditionals to handle control flow.
Also, not throwing errors is in keeping with JS's builtin library functions, which generally don't complain about invalid or missing input. For
insertAt
, for example, you could returntrue
if the insertion was successful andfalse
otherwise. If the user provides something silly as an index that causes a crash, they'll get an appropriate stack trace that likely explains the problem better than a hand-written error string.
If you do wish to stick with error throwing, ensure it is comprehensive. For example,
if (index > this.size)
doesn't handle the case whenindex < 0
which could result in difficult-to-track-down bugs for the client who has to make design assumptions based on your throw.
Then, once you've covered that scenario, it begs the question whether you should now validate that the provided input is an integer number and throw an error message for that as well.
The point is, adding errors gives the client the illusion of a comprehensive and robust set of argument and function state restrictions, which is problematic if they aren't actually robust. Throwing no errors, assuming the client is behaving, and reporting
true
/false
as to whether some function failed or not seems preferential.
Variable keywords
- Use
const
when appropriate in place oflet
. This should apply to almost everything except for loop counters, accumulator variables and runner nodes. For example,let out = ;
should beconst out = ;
. Even if the risk of bugs caused by accidental reassignment is low, this has semantic benefits.
Misleading function names
printList()
is a misleading name; it stringifies the list rather than printing. I recommend overridingtoString()
here.
insert()
usually refers to insertion at some specific element, which is the behavior yourinsertAt()
function offers. Withinsert()
, It's not obvious where the insertion is happening; one ofaddFront
,unshift
orpush
seem clearer, depending on which end of your list you decide the front to be.
Internal logic
insertAt
is not working correctly, placing items incorrectly, not at all, and neglecting to increment the size.- Consider adjusting your
insertAt
code to avoid usingreturn
s andcurr.next.next
, which is difficult to reason about and causing bugs. - In
printList
, pains are taken to conditionally add the->
arrow only for non-last elements when you can simply walk the list and useout.join("->")
. - Since your internal code relies only on
Node
objects, you can make your code cleaner by testingwhile (curr)
instead ofwhile (curr === null)
. This is debatable, because it restricts your internal logic from distinguishing betweennull
andundefined
or other falsey values, but if you trust yourself to be consistent about it, I prefer the cleaner look.
Interface
As written, I find your provided function interface insufficient for typical linked list needs. It's not obvious what functionality this class offers over, say, a primitive array.
Consider
remove(item)
. This sort of function that takes an element and searches the structure for it is best used for hashes with random access. The whole point of linked lists is to offer fast insertion and removal of front and back elements, regardless of what those elements might be. Anything in the middle is of no concern, and libraries that offer access to these elements, such as Java's LinkedList collection are generally considered to be flawed because clients may make false assumptions about the time complexity of provided operations.
Without constant time
front
andback
adds, removals and peeks, I can't foresee a reason to use this class instead of a primitive array.insert
is the only useful linked list function your interface offers (disregardingisEmpty
,printList
, etc as useful but trivial).
Rewrites
Revision #1 (same functionality)
class LinkedList {
constructor() {
this.head;
this.size = 0;
}
addFront(value) {
const newNode = new Node(value);
if (this.head) {
newNode.next = this.head;
}
this.head = newNode;
this.size++;
}
insertAt(value, idx) {
let curr = this.head;
let prev;
while (curr && idx > 0) {
prev = curr;
curr = curr.next;
idx--;
}
if (prev) {
prev.next = new Node(value);
prev.next.next = curr;
this.size++;
}
else {
this.addFront(value);
}
}
remove(value) {
let curr = this.head;
let prev;
while (curr) {
if (curr.val === value) {
if (prev) {
prev.next = curr.next;
}
else {
this.head = undefined;
}
this.size--;
return true;
}
prev = curr;
curr = curr.next;
}
return false;
}
length() {
return this.size;
}
empty() {
return !this.size;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());
Revision #2 (added front/back operations)
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());
Note that there is no popBack()
because this operation would be linear without doubly linked nodes. However, the class is sufficient to support both stacks and queues with all operations in constant time. Without the tail
pointer, only a stack could be supported. Adding popBack()
and double links would give you a deque class.
Revision #3 (supported a queue)
class Queue {
constructor() {
this.list = new LinkedList();
}
offer(value) {
this.list.addBack(value);
}
poll() {
return this.list.popFront();
}
peek() {
return this.list.peekFront();
}
empty() {
return this.list.empty();
}
toString() {
return this.list.toString();
}
}
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());
for (let i = 0; i < 5; i++) {
q.offer(i);
}
while (!q.empty()) {
console.log(q.poll());
}
After all that work, a benchmark shows that it's no faster than a primitive array. This is likely due to overhead from object creation, function calls and garbage collection, which counteracts the shifting necessary on the primitive array.
$endgroup$
1
$begingroup$
A delight to read! Dear OP, Look into iterators. That would enableforEach
coding. Also Blindman76 warning of lack of private variables inclass
is notable. You really want to protect clients from themselves and make sure the linkedList can be used only as intended.
$endgroup$
– radarbob
Jan 14 at 0:29
add a comment |
$begingroup$
TL;DR
- Style could better adhere to established standards.
insertAt
has bugs to resolve.- This class is missing basic add/remove functions which prevent it from being useful.
- Writing a linked list class in JS only makes sense from an educational standpoint; primitive arrays are efficient, universal and offer more functionality with less code.
Style
Spacing
Use a space between
)
s,{
s and keywords, e.g.
if(this.head === null){
is cleaner as
if (this.head === null) {
Add vertical spacing between lines to group logical code blocks together and separate loop, conditional and function logic from declarations. For example,
let curr = this.head;
while(curr !== null){
is easier to read as
let curr = this.head;
while (curr !== null) {
this.size ++
is clearer asthis.size++
.
Be consistent--sometimes you use
}else{
, and sometimes you use
}
else{
Your whitespace fixes can be done by putting the code into SE's Code Snippet and pressing "Tidy", then adding blank lines around blocks by hand. Generally, I don't see reason to deviate from this prescribed style.
Variable and function naming
- Function names switch between
snake_case
andcamelCase
. All JS naming should becamelCase
, except class names, which areUpperCamelCase
(as you use). - Avoid single-letter variable names like
p
unless the meaning is obvious. In this case, something likenewNode
might be clearer.
Functionality
Errors
throw "Wrong index";
is an unclear error message. What exactly is wrong with the index? Considerthrow "Index out of list bounds";
which more accurately describes the problem.
You may wish to reconsider using errors at all. I find throwing errors in JS generally less appropriate than return values because the calling code can stick to normal conditionals to handle control flow.
Also, not throwing errors is in keeping with JS's builtin library functions, which generally don't complain about invalid or missing input. For
insertAt
, for example, you could returntrue
if the insertion was successful andfalse
otherwise. If the user provides something silly as an index that causes a crash, they'll get an appropriate stack trace that likely explains the problem better than a hand-written error string.
If you do wish to stick with error throwing, ensure it is comprehensive. For example,
if (index > this.size)
doesn't handle the case whenindex < 0
which could result in difficult-to-track-down bugs for the client who has to make design assumptions based on your throw.
Then, once you've covered that scenario, it begs the question whether you should now validate that the provided input is an integer number and throw an error message for that as well.
The point is, adding errors gives the client the illusion of a comprehensive and robust set of argument and function state restrictions, which is problematic if they aren't actually robust. Throwing no errors, assuming the client is behaving, and reporting
true
/false
as to whether some function failed or not seems preferential.
Variable keywords
- Use
const
when appropriate in place oflet
. This should apply to almost everything except for loop counters, accumulator variables and runner nodes. For example,let out = ;
should beconst out = ;
. Even if the risk of bugs caused by accidental reassignment is low, this has semantic benefits.
Misleading function names
printList()
is a misleading name; it stringifies the list rather than printing. I recommend overridingtoString()
here.
insert()
usually refers to insertion at some specific element, which is the behavior yourinsertAt()
function offers. Withinsert()
, It's not obvious where the insertion is happening; one ofaddFront
,unshift
orpush
seem clearer, depending on which end of your list you decide the front to be.
Internal logic
insertAt
is not working correctly, placing items incorrectly, not at all, and neglecting to increment the size.- Consider adjusting your
insertAt
code to avoid usingreturn
s andcurr.next.next
, which is difficult to reason about and causing bugs. - In
printList
, pains are taken to conditionally add the->
arrow only for non-last elements when you can simply walk the list and useout.join("->")
. - Since your internal code relies only on
Node
objects, you can make your code cleaner by testingwhile (curr)
instead ofwhile (curr === null)
. This is debatable, because it restricts your internal logic from distinguishing betweennull
andundefined
or other falsey values, but if you trust yourself to be consistent about it, I prefer the cleaner look.
Interface
As written, I find your provided function interface insufficient for typical linked list needs. It's not obvious what functionality this class offers over, say, a primitive array.
Consider
remove(item)
. This sort of function that takes an element and searches the structure for it is best used for hashes with random access. The whole point of linked lists is to offer fast insertion and removal of front and back elements, regardless of what those elements might be. Anything in the middle is of no concern, and libraries that offer access to these elements, such as Java's LinkedList collection are generally considered to be flawed because clients may make false assumptions about the time complexity of provided operations.
Without constant time
front
andback
adds, removals and peeks, I can't foresee a reason to use this class instead of a primitive array.insert
is the only useful linked list function your interface offers (disregardingisEmpty
,printList
, etc as useful but trivial).
Rewrites
Revision #1 (same functionality)
class LinkedList {
constructor() {
this.head;
this.size = 0;
}
addFront(value) {
const newNode = new Node(value);
if (this.head) {
newNode.next = this.head;
}
this.head = newNode;
this.size++;
}
insertAt(value, idx) {
let curr = this.head;
let prev;
while (curr && idx > 0) {
prev = curr;
curr = curr.next;
idx--;
}
if (prev) {
prev.next = new Node(value);
prev.next.next = curr;
this.size++;
}
else {
this.addFront(value);
}
}
remove(value) {
let curr = this.head;
let prev;
while (curr) {
if (curr.val === value) {
if (prev) {
prev.next = curr.next;
}
else {
this.head = undefined;
}
this.size--;
return true;
}
prev = curr;
curr = curr.next;
}
return false;
}
length() {
return this.size;
}
empty() {
return !this.size;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());
Revision #2 (added front/back operations)
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());
Note that there is no popBack()
because this operation would be linear without doubly linked nodes. However, the class is sufficient to support both stacks and queues with all operations in constant time. Without the tail
pointer, only a stack could be supported. Adding popBack()
and double links would give you a deque class.
Revision #3 (supported a queue)
class Queue {
constructor() {
this.list = new LinkedList();
}
offer(value) {
this.list.addBack(value);
}
poll() {
return this.list.popFront();
}
peek() {
return this.list.peekFront();
}
empty() {
return this.list.empty();
}
toString() {
return this.list.toString();
}
}
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());
for (let i = 0; i < 5; i++) {
q.offer(i);
}
while (!q.empty()) {
console.log(q.poll());
}
After all that work, a benchmark shows that it's no faster than a primitive array. This is likely due to overhead from object creation, function calls and garbage collection, which counteracts the shifting necessary on the primitive array.
$endgroup$
TL;DR
- Style could better adhere to established standards.
insertAt
has bugs to resolve.- This class is missing basic add/remove functions which prevent it from being useful.
- Writing a linked list class in JS only makes sense from an educational standpoint; primitive arrays are efficient, universal and offer more functionality with less code.
Style
Spacing
Use a space between
)
s,{
s and keywords, e.g.
if(this.head === null){
is cleaner as
if (this.head === null) {
Add vertical spacing between lines to group logical code blocks together and separate loop, conditional and function logic from declarations. For example,
let curr = this.head;
while(curr !== null){
is easier to read as
let curr = this.head;
while (curr !== null) {
this.size ++
is clearer asthis.size++
.
Be consistent--sometimes you use
}else{
, and sometimes you use
}
else{
Your whitespace fixes can be done by putting the code into SE's Code Snippet and pressing "Tidy", then adding blank lines around blocks by hand. Generally, I don't see reason to deviate from this prescribed style.
Variable and function naming
- Function names switch between
snake_case
andcamelCase
. All JS naming should becamelCase
, except class names, which areUpperCamelCase
(as you use). - Avoid single-letter variable names like
p
unless the meaning is obvious. In this case, something likenewNode
might be clearer.
Functionality
Errors
throw "Wrong index";
is an unclear error message. What exactly is wrong with the index? Considerthrow "Index out of list bounds";
which more accurately describes the problem.
You may wish to reconsider using errors at all. I find throwing errors in JS generally less appropriate than return values because the calling code can stick to normal conditionals to handle control flow.
Also, not throwing errors is in keeping with JS's builtin library functions, which generally don't complain about invalid or missing input. For
insertAt
, for example, you could returntrue
if the insertion was successful andfalse
otherwise. If the user provides something silly as an index that causes a crash, they'll get an appropriate stack trace that likely explains the problem better than a hand-written error string.
If you do wish to stick with error throwing, ensure it is comprehensive. For example,
if (index > this.size)
doesn't handle the case whenindex < 0
which could result in difficult-to-track-down bugs for the client who has to make design assumptions based on your throw.
Then, once you've covered that scenario, it begs the question whether you should now validate that the provided input is an integer number and throw an error message for that as well.
The point is, adding errors gives the client the illusion of a comprehensive and robust set of argument and function state restrictions, which is problematic if they aren't actually robust. Throwing no errors, assuming the client is behaving, and reporting
true
/false
as to whether some function failed or not seems preferential.
Variable keywords
- Use
const
when appropriate in place oflet
. This should apply to almost everything except for loop counters, accumulator variables and runner nodes. For example,let out = ;
should beconst out = ;
. Even if the risk of bugs caused by accidental reassignment is low, this has semantic benefits.
Misleading function names
printList()
is a misleading name; it stringifies the list rather than printing. I recommend overridingtoString()
here.
insert()
usually refers to insertion at some specific element, which is the behavior yourinsertAt()
function offers. Withinsert()
, It's not obvious where the insertion is happening; one ofaddFront
,unshift
orpush
seem clearer, depending on which end of your list you decide the front to be.
Internal logic
insertAt
is not working correctly, placing items incorrectly, not at all, and neglecting to increment the size.- Consider adjusting your
insertAt
code to avoid usingreturn
s andcurr.next.next
, which is difficult to reason about and causing bugs. - In
printList
, pains are taken to conditionally add the->
arrow only for non-last elements when you can simply walk the list and useout.join("->")
. - Since your internal code relies only on
Node
objects, you can make your code cleaner by testingwhile (curr)
instead ofwhile (curr === null)
. This is debatable, because it restricts your internal logic from distinguishing betweennull
andundefined
or other falsey values, but if you trust yourself to be consistent about it, I prefer the cleaner look.
Interface
As written, I find your provided function interface insufficient for typical linked list needs. It's not obvious what functionality this class offers over, say, a primitive array.
Consider
remove(item)
. This sort of function that takes an element and searches the structure for it is best used for hashes with random access. The whole point of linked lists is to offer fast insertion and removal of front and back elements, regardless of what those elements might be. Anything in the middle is of no concern, and libraries that offer access to these elements, such as Java's LinkedList collection are generally considered to be flawed because clients may make false assumptions about the time complexity of provided operations.
Without constant time
front
andback
adds, removals and peeks, I can't foresee a reason to use this class instead of a primitive array.insert
is the only useful linked list function your interface offers (disregardingisEmpty
,printList
, etc as useful but trivial).
Rewrites
Revision #1 (same functionality)
class LinkedList {
constructor() {
this.head;
this.size = 0;
}
addFront(value) {
const newNode = new Node(value);
if (this.head) {
newNode.next = this.head;
}
this.head = newNode;
this.size++;
}
insertAt(value, idx) {
let curr = this.head;
let prev;
while (curr && idx > 0) {
prev = curr;
curr = curr.next;
idx--;
}
if (prev) {
prev.next = new Node(value);
prev.next.next = curr;
this.size++;
}
else {
this.addFront(value);
}
}
remove(value) {
let curr = this.head;
let prev;
while (curr) {
if (curr.val === value) {
if (prev) {
prev.next = curr.next;
}
else {
this.head = undefined;
}
this.size--;
return true;
}
prev = curr;
curr = curr.next;
}
return false;
}
length() {
return this.size;
}
empty() {
return !this.size;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());
Revision #2 (added front/back operations)
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());
Note that there is no popBack()
because this operation would be linear without doubly linked nodes. However, the class is sufficient to support both stacks and queues with all operations in constant time. Without the tail
pointer, only a stack could be supported. Adding popBack()
and double links would give you a deque class.
Revision #3 (supported a queue)
class Queue {
constructor() {
this.list = new LinkedList();
}
offer(value) {
this.list.addBack(value);
}
poll() {
return this.list.popFront();
}
peek() {
return this.list.peekFront();
}
empty() {
return this.list.empty();
}
toString() {
return this.list.toString();
}
}
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());
for (let i = 0; i < 5; i++) {
q.offer(i);
}
while (!q.empty()) {
console.log(q.poll());
}
After all that work, a benchmark shows that it's no faster than a primitive array. This is likely due to overhead from object creation, function calls and garbage collection, which counteracts the shifting necessary on the primitive array.
class LinkedList {
constructor() {
this.head;
this.size = 0;
}
addFront(value) {
const newNode = new Node(value);
if (this.head) {
newNode.next = this.head;
}
this.head = newNode;
this.size++;
}
insertAt(value, idx) {
let curr = this.head;
let prev;
while (curr && idx > 0) {
prev = curr;
curr = curr.next;
idx--;
}
if (prev) {
prev.next = new Node(value);
prev.next.next = curr;
this.size++;
}
else {
this.addFront(value);
}
}
remove(value) {
let curr = this.head;
let prev;
while (curr) {
if (curr.val === value) {
if (prev) {
prev.next = curr.next;
}
else {
this.head = undefined;
}
this.size--;
return true;
}
prev = curr;
curr = curr.next;
}
return false;
}
length() {
return this.size;
}
empty() {
return !this.size;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());
class LinkedList {
constructor() {
this.head;
this.size = 0;
}
addFront(value) {
const newNode = new Node(value);
if (this.head) {
newNode.next = this.head;
}
this.head = newNode;
this.size++;
}
insertAt(value, idx) {
let curr = this.head;
let prev;
while (curr && idx > 0) {
prev = curr;
curr = curr.next;
idx--;
}
if (prev) {
prev.next = new Node(value);
prev.next.next = curr;
this.size++;
}
else {
this.addFront(value);
}
}
remove(value) {
let curr = this.head;
let prev;
while (curr) {
if (curr.val === value) {
if (prev) {
prev.next = curr.next;
}
else {
this.head = undefined;
}
this.size--;
return true;
}
prev = curr;
curr = curr.next;
}
return false;
}
length() {
return this.size;
}
empty() {
return !this.size;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());
class Queue {
constructor() {
this.list = new LinkedList();
}
offer(value) {
this.list.addBack(value);
}
poll() {
return this.list.popFront();
}
peek() {
return this.list.peekFront();
}
empty() {
return this.list.empty();
}
toString() {
return this.list.toString();
}
}
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());
for (let i = 0; i < 5; i++) {
q.offer(i);
}
while (!q.empty()) {
console.log(q.poll());
}
class Queue {
constructor() {
this.list = new LinkedList();
}
offer(value) {
this.list.addBack(value);
}
poll() {
return this.list.popFront();
}
peek() {
return this.list.peekFront();
}
empty() {
return this.list.empty();
}
toString() {
return this.list.toString();
}
}
class LinkedList {
constructor() {
this.head;
this.tail;
}
addFront(value) {
const newHead = new Node(value);
if (this.head) {
newHead.next = this.head;
}
else {
this.tail = newHead;
}
this.head = newHead;
}
addBack(value) {
if (this.tail) {
this.tail.next = new Node(value);
this.tail = this.tail.next;
}
else {
this.head = this.tail = new Node(value);
}
}
peekFront() {
return this.head ? this.head.val : null;
}
peekBack() {
return this.tail ? this.tail.val : null;
}
popFront() {
if (this.head) {
const value = this.head.val;
this.head = this.head.next;
if (!this.head) {
this.tail = null;
}
return value;
}
}
empty() {
return !this.head;
}
toString() {
const result = ;
let curr = this.head;
while (curr) {
result.push(curr.val);
curr = curr.next;
}
return result.join("->");
}
}
class Node {
constructor(val, nextNode=null) {
this.val = val;
this.next = nextNode;
}
}
const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());
for (let i = 0; i < 5; i++) {
q.offer(i);
}
while (!q.empty()) {
console.log(q.poll());
}
edited Jan 13 at 0:11
answered Jan 12 at 20:56
ggorlenggorlen
3738
3738
1
$begingroup$
A delight to read! Dear OP, Look into iterators. That would enableforEach
coding. Also Blindman76 warning of lack of private variables inclass
is notable. You really want to protect clients from themselves and make sure the linkedList can be used only as intended.
$endgroup$
– radarbob
Jan 14 at 0:29
add a comment |
1
$begingroup$
A delight to read! Dear OP, Look into iterators. That would enableforEach
coding. Also Blindman76 warning of lack of private variables inclass
is notable. You really want to protect clients from themselves and make sure the linkedList can be used only as intended.
$endgroup$
– radarbob
Jan 14 at 0:29
1
1
$begingroup$
A delight to read! Dear OP, Look into iterators. That would enable
forEach
coding. Also Blindman76 warning of lack of private variables in class
is notable. You really want to protect clients from themselves and make sure the linkedList can be used only as intended.$endgroup$
– radarbob
Jan 14 at 0:29
$begingroup$
A delight to read! Dear OP, Look into iterators. That would enable
forEach
coding. Also Blindman76 warning of lack of private variables in class
is notable. You really want to protect clients from themselves and make sure the linkedList can be used only as intended.$endgroup$
– radarbob
Jan 14 at 0:29
add a comment |
$begingroup$
Unsafe and unusable.
Class syntax is dangerous
The class syntax encourages you to expose the objects state, this means it can mutate outside your control.
New developments allow class to have private properties. #privateNamed
the hash means private. However its implementations is an abomination to the languages, we now have a situation where access type is embedded in the variable name. Names should be independent of any language constructs. Anyways I digress...
As it stands your object (class
) is unsafe, you expose head
and size
(in LinkedList
) and next
(in Node
) meaning that outside code can deliberately or by accident mutate the objects state such that your function becomes inoperable.
It is possible for your code to indefinitely block the page, meaning that the only way to fix the problem is for the page to crash or it to be forced to close.
Mutation examples
const list = new LinkedList()
list.insert("😧");
list.head.next = list.head; // cyclic link.
list.remove("☠"); // Untrappable page blocking error
list.printList(); // will crash the page with out of memory error
const list = new LinkedList()
list.size = "🙈🙉🙊";
console.log(list.length); // >> "🙈🙉🙊" nonsense
const list = new LinkedList()
list.insert("A");
const a = list.head;
list.insert("B");
const b = list.head;
list.insert("C");
list.insert("D");
const d = list.head;
b.next = d; // removes c
console.log(list.length()); // >> 4 Incorrect
var node = a;
for(let i = 0; i < list.length; i++){
console.log(node.value); // WTF throws error on 4th iteration
node = node.next;
}
Normal use errors
const list = new LinkedList()
list.insert("A");
list.insertAt("B", -1); // does not insert returning undefined.
const list = new LinkedList()
list.size = 100;
console.log(list.is__Empty()); // >> false, wrong answer the list is empty
I could go on, there are many dangers when you expose internal state. Programmers, you included will be tempted to use a shortcuts, or accidently mutate the list with catastrophic consequences.
I would never allow such a dangerous object into a project, it is unusable because of its dangerous behaviour.
Object factories
Consider using a factory to create your Object.
Factories let you create a private state via closure that can not be mutate. You can confidently use the state because it is hidden and immutable.
Factory example.
The factory returns a frozen object with the state hidden via closure. I add to functions. Iterator LinkedList.values
to iterate the list from 0
to size-1
eg console.log([...list.values()])
will list items as an array. Also linkedList.itemAt(index)
as the linked list is useless without them.
You do not get access to nodes, only the values they contain. And printList
is called toString
Note code is untested and as an example only. May contain typos or logic errors.
function LinkedList() {
var head, size = 0;
const add = (value, next) => {
size ++;
return {value, next};
}
const vetIndex = idx => isNaN(idx) || idx > size || idx < 0;
const list = Object.freeze({
get length() { return size },
get isEmpty() { return size === 0 },
insert(item) { head = add(item, head) },
insertAt(item, idx = 0) {
if (vetIndex(idx)) { throw new RangeError("Bad index") }
if (idx === size) { head = add(item, head) }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
curr.next = add(item, curr.next);
}
},
remove(item) {
if (head.value === item) {
head = head.next;
size --;
} else {
let curr = head;
while (curr && curr.next && curr.next.value !== item) { curr = curr.next }
if (curr) {
curr.next = curr.next ? curr.next.next : undefined;
size --;
}
}
},
toString() {
var str = "";
if (size) {
str += head.value;
let curr = head.next;
while (curr) {
str += "->" + curr.value;
curr = curr.next;
}
}
return str;
},
*values() {
var idx = 0;
while (idx < size) { yield list.itemAt(idx++) }
},
itemAt(idx) {
if (size && !vetIndex(idx)) {
vetIndex(idx);
if (idx === size) { return head.value }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
return curr.value
}
}
},
});
return list;
}
Some more points on your code
- Function scope variables should be declared as
var
. Show you understand the language and use the correct declaration type. - Use
const
for constants. Eg inprintList
you define an arraylet out = ;
out
is a reference to an array, the reference never changes, only the content of the array does, so use a constantconst out = ;
- Only throw if not doing so will damage state such that continuing will create undefined behaviours. You threw if an item could not be found in
remove
There is no reason to throw as it does not damage your state. Returnundefined
and let the calling function deal with their problems. - When you throw do not throw strings (many catches assume an object and rethrow if its just a string). Use appropriate error objects. eg you throw a string
throw "Wrong index";
you should throw an errorthrow new RangeError("Bad index");
Orthrow new Error("Bad index");
- Don't add redundant code. the function
insertAt
has 3 returns yet can be written with on extraelse
and abreak
, with noreturn
tokens. - Don't use
null
, itsundefined
if not defined. - Always use the shorts form. Eg
if(this.head === null)
useif(!this.head)
andif (!curr.next) { curr.next = p;} else if (!curr.next.next) { curr.next = p;}
becomesif(!curr.next || !curr.next..next) { curr.next = p }
$endgroup$
add a comment |
$begingroup$
Unsafe and unusable.
Class syntax is dangerous
The class syntax encourages you to expose the objects state, this means it can mutate outside your control.
New developments allow class to have private properties. #privateNamed
the hash means private. However its implementations is an abomination to the languages, we now have a situation where access type is embedded in the variable name. Names should be independent of any language constructs. Anyways I digress...
As it stands your object (class
) is unsafe, you expose head
and size
(in LinkedList
) and next
(in Node
) meaning that outside code can deliberately or by accident mutate the objects state such that your function becomes inoperable.
It is possible for your code to indefinitely block the page, meaning that the only way to fix the problem is for the page to crash or it to be forced to close.
Mutation examples
const list = new LinkedList()
list.insert("😧");
list.head.next = list.head; // cyclic link.
list.remove("☠"); // Untrappable page blocking error
list.printList(); // will crash the page with out of memory error
const list = new LinkedList()
list.size = "🙈🙉🙊";
console.log(list.length); // >> "🙈🙉🙊" nonsense
const list = new LinkedList()
list.insert("A");
const a = list.head;
list.insert("B");
const b = list.head;
list.insert("C");
list.insert("D");
const d = list.head;
b.next = d; // removes c
console.log(list.length()); // >> 4 Incorrect
var node = a;
for(let i = 0; i < list.length; i++){
console.log(node.value); // WTF throws error on 4th iteration
node = node.next;
}
Normal use errors
const list = new LinkedList()
list.insert("A");
list.insertAt("B", -1); // does not insert returning undefined.
const list = new LinkedList()
list.size = 100;
console.log(list.is__Empty()); // >> false, wrong answer the list is empty
I could go on, there are many dangers when you expose internal state. Programmers, you included will be tempted to use a shortcuts, or accidently mutate the list with catastrophic consequences.
I would never allow such a dangerous object into a project, it is unusable because of its dangerous behaviour.
Object factories
Consider using a factory to create your Object.
Factories let you create a private state via closure that can not be mutate. You can confidently use the state because it is hidden and immutable.
Factory example.
The factory returns a frozen object with the state hidden via closure. I add to functions. Iterator LinkedList.values
to iterate the list from 0
to size-1
eg console.log([...list.values()])
will list items as an array. Also linkedList.itemAt(index)
as the linked list is useless without them.
You do not get access to nodes, only the values they contain. And printList
is called toString
Note code is untested and as an example only. May contain typos or logic errors.
function LinkedList() {
var head, size = 0;
const add = (value, next) => {
size ++;
return {value, next};
}
const vetIndex = idx => isNaN(idx) || idx > size || idx < 0;
const list = Object.freeze({
get length() { return size },
get isEmpty() { return size === 0 },
insert(item) { head = add(item, head) },
insertAt(item, idx = 0) {
if (vetIndex(idx)) { throw new RangeError("Bad index") }
if (idx === size) { head = add(item, head) }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
curr.next = add(item, curr.next);
}
},
remove(item) {
if (head.value === item) {
head = head.next;
size --;
} else {
let curr = head;
while (curr && curr.next && curr.next.value !== item) { curr = curr.next }
if (curr) {
curr.next = curr.next ? curr.next.next : undefined;
size --;
}
}
},
toString() {
var str = "";
if (size) {
str += head.value;
let curr = head.next;
while (curr) {
str += "->" + curr.value;
curr = curr.next;
}
}
return str;
},
*values() {
var idx = 0;
while (idx < size) { yield list.itemAt(idx++) }
},
itemAt(idx) {
if (size && !vetIndex(idx)) {
vetIndex(idx);
if (idx === size) { return head.value }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
return curr.value
}
}
},
});
return list;
}
Some more points on your code
- Function scope variables should be declared as
var
. Show you understand the language and use the correct declaration type. - Use
const
for constants. Eg inprintList
you define an arraylet out = ;
out
is a reference to an array, the reference never changes, only the content of the array does, so use a constantconst out = ;
- Only throw if not doing so will damage state such that continuing will create undefined behaviours. You threw if an item could not be found in
remove
There is no reason to throw as it does not damage your state. Returnundefined
and let the calling function deal with their problems. - When you throw do not throw strings (many catches assume an object and rethrow if its just a string). Use appropriate error objects. eg you throw a string
throw "Wrong index";
you should throw an errorthrow new RangeError("Bad index");
Orthrow new Error("Bad index");
- Don't add redundant code. the function
insertAt
has 3 returns yet can be written with on extraelse
and abreak
, with noreturn
tokens. - Don't use
null
, itsundefined
if not defined. - Always use the shorts form. Eg
if(this.head === null)
useif(!this.head)
andif (!curr.next) { curr.next = p;} else if (!curr.next.next) { curr.next = p;}
becomesif(!curr.next || !curr.next..next) { curr.next = p }
$endgroup$
add a comment |
$begingroup$
Unsafe and unusable.
Class syntax is dangerous
The class syntax encourages you to expose the objects state, this means it can mutate outside your control.
New developments allow class to have private properties. #privateNamed
the hash means private. However its implementations is an abomination to the languages, we now have a situation where access type is embedded in the variable name. Names should be independent of any language constructs. Anyways I digress...
As it stands your object (class
) is unsafe, you expose head
and size
(in LinkedList
) and next
(in Node
) meaning that outside code can deliberately or by accident mutate the objects state such that your function becomes inoperable.
It is possible for your code to indefinitely block the page, meaning that the only way to fix the problem is for the page to crash or it to be forced to close.
Mutation examples
const list = new LinkedList()
list.insert("😧");
list.head.next = list.head; // cyclic link.
list.remove("☠"); // Untrappable page blocking error
list.printList(); // will crash the page with out of memory error
const list = new LinkedList()
list.size = "🙈🙉🙊";
console.log(list.length); // >> "🙈🙉🙊" nonsense
const list = new LinkedList()
list.insert("A");
const a = list.head;
list.insert("B");
const b = list.head;
list.insert("C");
list.insert("D");
const d = list.head;
b.next = d; // removes c
console.log(list.length()); // >> 4 Incorrect
var node = a;
for(let i = 0; i < list.length; i++){
console.log(node.value); // WTF throws error on 4th iteration
node = node.next;
}
Normal use errors
const list = new LinkedList()
list.insert("A");
list.insertAt("B", -1); // does not insert returning undefined.
const list = new LinkedList()
list.size = 100;
console.log(list.is__Empty()); // >> false, wrong answer the list is empty
I could go on, there are many dangers when you expose internal state. Programmers, you included will be tempted to use a shortcuts, or accidently mutate the list with catastrophic consequences.
I would never allow such a dangerous object into a project, it is unusable because of its dangerous behaviour.
Object factories
Consider using a factory to create your Object.
Factories let you create a private state via closure that can not be mutate. You can confidently use the state because it is hidden and immutable.
Factory example.
The factory returns a frozen object with the state hidden via closure. I add to functions. Iterator LinkedList.values
to iterate the list from 0
to size-1
eg console.log([...list.values()])
will list items as an array. Also linkedList.itemAt(index)
as the linked list is useless without them.
You do not get access to nodes, only the values they contain. And printList
is called toString
Note code is untested and as an example only. May contain typos or logic errors.
function LinkedList() {
var head, size = 0;
const add = (value, next) => {
size ++;
return {value, next};
}
const vetIndex = idx => isNaN(idx) || idx > size || idx < 0;
const list = Object.freeze({
get length() { return size },
get isEmpty() { return size === 0 },
insert(item) { head = add(item, head) },
insertAt(item, idx = 0) {
if (vetIndex(idx)) { throw new RangeError("Bad index") }
if (idx === size) { head = add(item, head) }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
curr.next = add(item, curr.next);
}
},
remove(item) {
if (head.value === item) {
head = head.next;
size --;
} else {
let curr = head;
while (curr && curr.next && curr.next.value !== item) { curr = curr.next }
if (curr) {
curr.next = curr.next ? curr.next.next : undefined;
size --;
}
}
},
toString() {
var str = "";
if (size) {
str += head.value;
let curr = head.next;
while (curr) {
str += "->" + curr.value;
curr = curr.next;
}
}
return str;
},
*values() {
var idx = 0;
while (idx < size) { yield list.itemAt(idx++) }
},
itemAt(idx) {
if (size && !vetIndex(idx)) {
vetIndex(idx);
if (idx === size) { return head.value }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
return curr.value
}
}
},
});
return list;
}
Some more points on your code
- Function scope variables should be declared as
var
. Show you understand the language and use the correct declaration type. - Use
const
for constants. Eg inprintList
you define an arraylet out = ;
out
is a reference to an array, the reference never changes, only the content of the array does, so use a constantconst out = ;
- Only throw if not doing so will damage state such that continuing will create undefined behaviours. You threw if an item could not be found in
remove
There is no reason to throw as it does not damage your state. Returnundefined
and let the calling function deal with their problems. - When you throw do not throw strings (many catches assume an object and rethrow if its just a string). Use appropriate error objects. eg you throw a string
throw "Wrong index";
you should throw an errorthrow new RangeError("Bad index");
Orthrow new Error("Bad index");
- Don't add redundant code. the function
insertAt
has 3 returns yet can be written with on extraelse
and abreak
, with noreturn
tokens. - Don't use
null
, itsundefined
if not defined. - Always use the shorts form. Eg
if(this.head === null)
useif(!this.head)
andif (!curr.next) { curr.next = p;} else if (!curr.next.next) { curr.next = p;}
becomesif(!curr.next || !curr.next..next) { curr.next = p }
$endgroup$
Unsafe and unusable.
Class syntax is dangerous
The class syntax encourages you to expose the objects state, this means it can mutate outside your control.
New developments allow class to have private properties. #privateNamed
the hash means private. However its implementations is an abomination to the languages, we now have a situation where access type is embedded in the variable name. Names should be independent of any language constructs. Anyways I digress...
As it stands your object (class
) is unsafe, you expose head
and size
(in LinkedList
) and next
(in Node
) meaning that outside code can deliberately or by accident mutate the objects state such that your function becomes inoperable.
It is possible for your code to indefinitely block the page, meaning that the only way to fix the problem is for the page to crash or it to be forced to close.
Mutation examples
const list = new LinkedList()
list.insert("😧");
list.head.next = list.head; // cyclic link.
list.remove("☠"); // Untrappable page blocking error
list.printList(); // will crash the page with out of memory error
const list = new LinkedList()
list.size = "🙈🙉🙊";
console.log(list.length); // >> "🙈🙉🙊" nonsense
const list = new LinkedList()
list.insert("A");
const a = list.head;
list.insert("B");
const b = list.head;
list.insert("C");
list.insert("D");
const d = list.head;
b.next = d; // removes c
console.log(list.length()); // >> 4 Incorrect
var node = a;
for(let i = 0; i < list.length; i++){
console.log(node.value); // WTF throws error on 4th iteration
node = node.next;
}
Normal use errors
const list = new LinkedList()
list.insert("A");
list.insertAt("B", -1); // does not insert returning undefined.
const list = new LinkedList()
list.size = 100;
console.log(list.is__Empty()); // >> false, wrong answer the list is empty
I could go on, there are many dangers when you expose internal state. Programmers, you included will be tempted to use a shortcuts, or accidently mutate the list with catastrophic consequences.
I would never allow such a dangerous object into a project, it is unusable because of its dangerous behaviour.
Object factories
Consider using a factory to create your Object.
Factories let you create a private state via closure that can not be mutate. You can confidently use the state because it is hidden and immutable.
Factory example.
The factory returns a frozen object with the state hidden via closure. I add to functions. Iterator LinkedList.values
to iterate the list from 0
to size-1
eg console.log([...list.values()])
will list items as an array. Also linkedList.itemAt(index)
as the linked list is useless without them.
You do not get access to nodes, only the values they contain. And printList
is called toString
Note code is untested and as an example only. May contain typos or logic errors.
function LinkedList() {
var head, size = 0;
const add = (value, next) => {
size ++;
return {value, next};
}
const vetIndex = idx => isNaN(idx) || idx > size || idx < 0;
const list = Object.freeze({
get length() { return size },
get isEmpty() { return size === 0 },
insert(item) { head = add(item, head) },
insertAt(item, idx = 0) {
if (vetIndex(idx)) { throw new RangeError("Bad index") }
if (idx === size) { head = add(item, head) }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
curr.next = add(item, curr.next);
}
},
remove(item) {
if (head.value === item) {
head = head.next;
size --;
} else {
let curr = head;
while (curr && curr.next && curr.next.value !== item) { curr = curr.next }
if (curr) {
curr.next = curr.next ? curr.next.next : undefined;
size --;
}
}
},
toString() {
var str = "";
if (size) {
str += head.value;
let curr = head.next;
while (curr) {
str += "->" + curr.value;
curr = curr.next;
}
}
return str;
},
*values() {
var idx = 0;
while (idx < size) { yield list.itemAt(idx++) }
},
itemAt(idx) {
if (size && !vetIndex(idx)) {
vetIndex(idx);
if (idx === size) { return head.value }
else {
let curr = head;
while (++idx < size) { curr = curr.next }
return curr.value
}
}
},
});
return list;
}
Some more points on your code
- Function scope variables should be declared as
var
. Show you understand the language and use the correct declaration type. - Use
const
for constants. Eg inprintList
you define an arraylet out = ;
out
is a reference to an array, the reference never changes, only the content of the array does, so use a constantconst out = ;
- Only throw if not doing so will damage state such that continuing will create undefined behaviours. You threw if an item could not be found in
remove
There is no reason to throw as it does not damage your state. Returnundefined
and let the calling function deal with their problems. - When you throw do not throw strings (many catches assume an object and rethrow if its just a string). Use appropriate error objects. eg you throw a string
throw "Wrong index";
you should throw an errorthrow new RangeError("Bad index");
Orthrow new Error("Bad index");
- Don't add redundant code. the function
insertAt
has 3 returns yet can be written with on extraelse
and abreak
, with noreturn
tokens. - Don't use
null
, itsundefined
if not defined. - Always use the shorts form. Eg
if(this.head === null)
useif(!this.head)
andif (!curr.next) { curr.next = p;} else if (!curr.next.next) { curr.next = p;}
becomesif(!curr.next || !curr.next..next) { curr.next = p }
answered Jan 13 at 20:49
Blindman67Blindman67
7,4911521
7,4911521
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211380%2fsingly-linked-list-implementation-in-es6%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown