Add SinglyLinkedList Data Structure Class #1

rfearing · 2021-10-29T11:27:45Z

Work Done

Added a SinglyLinkedList class
Added a SinglyLinkedListNode class, to be utilized in SinglyLinkedList class
Added Jest tests for the newly created SinglyLinkedList class

Note, this is a bigger PR than originally intended, but each method is broken out into a commit.

What I'd like in a review:

Does the logic itself make sense and is it clear?
Would you do it a different way?
Is there anything you'd add or do differently?
Is there anything in the initial setup: Jest, NPM, Typescript etc, that you'd choose differently?

gtempus

Thank you so much for letting me be a part of your project, @rfearing! I hope I added a couple of worthwhile things to think about with regards to data structures and OOD.

gtempus · 2021-11-01T14:17:58Z

README.md

 - [ ] Hash Table
 - [ ] Graph

+I'm utilizing the [Wiki page as a "lessons learned" section][lessons-learned]. Hopefully it'll help future me, and whoever stumbles upon this repo.


Nice! I need to get into a better habit of doing this. It'll be cool to see what this gets filled with.

gtempus · 2021-11-01T14:19:30Z

src/singlyLinkedList/README.md

+> [A singly linked list] is the simplest type of linked list in which every node contains some data and a pointer to the next node of the same data type. The node contains a pointer to the next node means that the node stores the address of the next node in the sequence. A single linked list allows traversal of data only in one way
+
+<img src="../../assets/singly-linked-list.png" width="300" alt="Graph representation of a singly linked list" />


Including a definition and diagram is great! 🤓

Is there any plan to compare/contrast with existing Javascript data structures? For example, were you going to do any research into how Array or TypedArray is implemented and compare that to what you have? Any interest in doing performance analysis with a profiler, etc?

With respect to implementation and constraints, it would be nice to give an example of where a dynamically allocated array is useful. Maybe what types of environments it's best in? Trade-offs with space vs time? Big O analysis?

I don't think you have to have that stuff. I'm just making sure I know what all your goals are for this project.

gtempus · 2021-11-01T14:28:41Z

src/singlyLinkedList/index.ts

@@ -0,0 +1,217 @@
+import { SinglyLinkedNode as Node } from './node';
+
+export class SinglyLinkedList<T> {


This is a full-featured SLL! 💪 Are you attempting to match the interface of Array or just picking interesting functionality?

I've been going through this Udemy course and have been adopting / modifying the methods the instructor teaches. One thing tho, I think I may change the method names to be more descriptive. E.g. insertAtEnd instead of push OR at least make aliases for those methods. Even in working on the class I had to keep asking which is which hahah. Any thoughts on method names?

gtempus · 2021-11-01T14:44:25Z

src/singlyLinkedList/index.ts

+	 * @param {number} index - Where the node is located in the list.
+	 * @returns {SinglyLinkedNode | null}
+	 */
+	get(index: number) {


I would recommend not returning a SinglyLinkedNode for all your getters. Assuming a client that is interested in storing objects in a data structure and not concerned with the how, your implementation actually breaks encapsulation. Clients are now aware of a SinglyLinkedNode and will use it with unintended consequences. I would just return the value that the client entrusted to you. This also has the beauty of preserving symmetry with the classes setters:

gtempus · 2021-11-01T14:51:55Z

src/singlyLinkedList/index.ts

+			this.head = newNode;
+			this.tail = this.head;
+		} else {
+			this.tail?.setNext(newNode);


I'm not a fan of optional chaining when the implementation is completely controlled by the creator/implementer of the class and not its clients. This alone is a good exercise for software design. Make sure you clearly state the invariants of each interaction (method, function, etc). Is it okay that this class isn't sure that tail is non-null when it also claims to be not empty? hmmm. A good way to track invariants is to use assert in your methods where you expect those invariants to hold for every single occasion. There is a good discussion of this in Fowler's Refactoring 2nd Ed. if you're curious.

@gtempus I just read the Introduce Assertion portion you mentioned. This is definitely going into a "Lessons Learned" section.

I was thinking "man, I don't love that I have to do optional chaining here, but TS keeps complaining".

Even with an assert I'm getting the same TS error. Maybe I need to change my tsconfig or maybe there's something else you can suggest? Would love to pair too on maybe removing one of these optional chains?

I actually just realized... I couuuuld optionally chain in the assertion. Thoughts?

gtempus · 2021-11-01T14:54:11Z

tests/singlyLinkedList.test.ts

+test('SinglyLinkedList push method correctly adds to the end of the list', () => {
+	expect(newList.head).toBeNull();
+	newList.push('1');
+	newList.push('2');
+	newList.push({ hello: 'world' });
+	expect(newList.head?.getValue()).toEqual('1');
+	expect(newList.head?.getNext()?.getValue()).toEqual('2');
+	expect(newList.tail?.getValue()).toEqual({ hello: 'world' });
+	expect(newList.length).toEqual(3);
+});


I love all the tests but I would encourage you to break them up. Too many expects lead to tests breaking all the time with the smallest of changes.

Also, I'm not sure it's a good idea to allow access to head and tail. Again, it breaks encapsulation and opens your nice SLL to all sorts of abuses. Testing the implememtation and not the interface will also make your tests more brittle.

I haven't tried these javascript features yet: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields

rfearing · 2021-11-27T23:34:51Z

@gtempus Thanks for this feedback. I made some of the changes we discussed. I'll probably do a comparison between the structures closer to the end. Care to give it another peek?

townofdon · 2021-11-29T18:22:23Z

src/singlyLinkedList/node.ts

+  }
+
+  next: SinglyLinkedNode<T> | null;
+  value: T | null;


Since you're using Typescript, you can add private access modifier to prevent accidentally writing directly to next or value when using a LinkedList node

Love this! Thank you!

townofdon · 2021-11-29T18:24:21Z

tests/singlyLinkedList.test.ts

+
+	describe('.getLength', () => {
+		it('return the length of the list', () => {
+			expect(newList.getLength()).toEqual(2);


Might suggest a beforeEach method as a cleanup to isolate all the tests from each other, making them more deterministic. (This test case depends on a previous test case)

Good call. Made that change :-D

townofdon · 2021-11-29T18:32:33Z

Looks good! I agree with @gtempus 's comments above (we both made a similar observation about adding private access modifiers haha)

Additionally, as a personal preference, I like to include usage docs whenever possible, but since this is a learning exercise, having the tests be the place to show off the functionality may suffice.

Also, I noticed that you're using JSDocs, which are great - Typescript does allow for some auto doc functionality however by including the return type of a function.

const myFunc = (): ReturnType => { ... }

In VSCode at least, hovering over myFunc anywhere in the codebase would should that it has a return value of ReturnType.

* Add SinglyLinkedList with push method * Add Pop to SinglyLinkedList * Add lessons learned to README * Add todo methods and snub tests * Add shift method to SinglyLinkedList * Add unshift method to SinglyLinkedList class * Add get method to SinglyLinkedList * Add link to SinglyLinkedList README * Add set method to SinglyLinkedList * Add insert and remove methods to SinglyLinkedList * Add reverse method to SLL * Make methods private and remove optional chaining * Update method names * Update tests * Add list v array to README * Update class to use TS private modifier * Update tests to utilize beforeEach

Ricardo Fearing added 3 commits October 29, 2021 07:26

Add SinglyLinkedList with push method

a9578e4

Add Pop to SinglyLinkedList

1b3b662

Add lessons learned to README

a749885

rfearing marked this pull request as ready for review October 29, 2021 12:55

rfearing requested a review from gtempus October 29, 2021 12:55

Ricardo Fearing added 6 commits October 29, 2021 09:43

Add todo methods and snub tests

0222951

Add shift method to SinglyLinkedList

1f02a7e

Add unshift method to SinglyLinkedList class

249ce91

Add get method to SinglyLinkedList

99e8457

Add link to SinglyLinkedList README

4df7f40

Add set method to SinglyLinkedList

6fbf0a7

rfearing force-pushed the add-singly-linked-list branch 2 times, most recently from a39683a to 4214c3a Compare October 29, 2021 17:25

Add insert and remove methods to SinglyLinkedList

24568a7

rfearing force-pushed the add-singly-linked-list branch from 4214c3a to 24568a7 Compare October 29, 2021 17:30

Add reverse method to SLL

b1f12a7

rfearing force-pushed the add-singly-linked-list branch from 597dfaf to b1f12a7 Compare October 29, 2021 17:57

rfearing changed the title ~~Add singly linked list~~ Add SinglyLinkedList Data Structure Class Oct 29, 2021

gtempus reviewed Nov 1, 2021

View reviewed changes

Ricardo Fearing added 3 commits November 27, 2021 13:58

Make methods private and remove optional chaining

e5d05a3

Update method names

a0cf910

Update tests

736625a

rfearing force-pushed the add-singly-linked-list branch from 37ebceb to 736625a Compare November 27, 2021 22:09

Add list v array to README

1109ae0

townofdon reviewed Nov 29, 2021

View reviewed changes

Ricardo Fearing added 2 commits December 9, 2021 15:30

Update class to use TS private modifier

7487805

Update tests to utilize beforeEach

3accaec

rfearing merged commit 11ac939 into main Dec 9, 2021

rfearing deleted the add-singly-linked-list branch December 9, 2021 20:50

		> [A singly linked list] is the simplest type of linked list in which every node contains some data and a pointer to the next node of the same data type. The node contains a pointer to the next node means that the node stores the address of the next node in the sequence. A single linked list allows traversal of data only in one way

		<img src="../../assets/singly-linked-list.png" width="300" alt="Graph representation of a singly linked list" />

		@@ -0,0 +1,217 @@
		import { SinglyLinkedNode as Node } from './node';

		export class SinglyLinkedList<T> {

Add SinglyLinkedList Data Structure Class #1

Add SinglyLinkedList Data Structure Class #1

Uh oh!

Conversation

rfearing commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work Done

What I'd like in a review:

Uh oh!

gtempus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfearing commented Nov 27, 2021

Uh oh!

townofdon Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

townofdon commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rfearing commented Oct 29, 2021 •

edited

Loading

townofdon Nov 29, 2021 •

edited

Loading