Step 2 - Implementing Functions #10

michmitz · 2021-12-17T20:21:56Z

1. What this does:

Implements CRUD functions for user class:
- Get user by id
- Get all users
- Create user
- Delete user
- Update user
Creates CustomError class with customized status code and message
Tests edge cases for each function, such as when no user can be found by id.

2. Ticket:
Discussed with Aron.

3. How to test:
-All tests are currently passing.

ianandrewson

Super solid. The only thing I would change are the status code if you want to, and not instantiating a new class in your create and delete methods.

ianandrewson · 2021-12-20T20:28:59Z

src/index.test.ts

+      } catch (error: any) {
+          expect(error).toBeInstanceOf(CustomError)
+          expect(error.message).toBe("No user found.")
+          expect(error.status).toEqual(500)


I'm no expert in this, but I'm not sure status 500 is the most semantic status you can use here. On the one hand, it is the server running the code to find the user. On the other hand, it's a specific resource that can't be found, likely queried by the client. Aron should probably weigh in on what the best status codes to use here are. Personally, I implemented error codes in the 400 range.

ianandrewson · 2021-12-20T20:32:48Z

src/index.test.ts

@@ -55,13 +86,20 @@ describe('Tests will go here!', () => {
    ]) 
  }) 

-  it.skip('returns null if no users are found', () => {
-      const result = users.getUserById(10)
+  it('returns CustomError if no users are found', () => {


The behavior here would probably be defined by a product owner or someone other than you or I, but there is a semantic argument to be made that when getting all users, returning an empty is totally fine instead of returning an error. Technically, all users were found, there just aren't any. And as long as you return an empty array and not null, any front end implementation shouldn't choke on an empty array.

So the question becomes, is finding no users an error?

ianandrewson · 2021-12-21T00:23:34Z

src/index.test.ts

+    } catch (error: any) {
+        expect(error).toBeInstanceOf(CustomError)
+        expect(error.message).toBe("No users found.")
+        expect(error.status).toEqual(500)


A more specific status code can likely be given here as well.

ianandrewson · 2021-12-21T00:23:48Z

src/index.test.ts

+    } catch (error: any) {
+        expect(error).toBeInstanceOf(CustomError)
+        expect(error.message).toBe("User with id already exists.")
+        expect(error.status).toEqual(500)


ianandrewson · 2021-12-21T00:24:14Z

src/index.test.ts

+      } catch (error: any) {
+          expect(error).toBeInstanceOf(CustomError)
+          expect(error.message).toBe("No user with that id found.")
+          expect(error.status).toEqual(500)


ianandrewson · 2021-12-21T00:24:47Z

src/index.test.ts

+      } catch (error: any) {
+          expect(error).toBeInstanceOf(CustomError)
+          expect(error.message).toBe("Different user with same id already exists.")
+          expect(error.status).toEqual(500)


Yup, and here.

ianandrewson · 2021-12-21T00:24:56Z

src/index.test.ts

+      } catch (error: any) {
+          expect(error).toBeInstanceOf(CustomError)
+          expect(error.message).toBe("No user found by that id.")
+          expect(error.status).toEqual(500)


The last one.

ianandrewson · 2021-12-21T00:26:10Z

src/index.ts

+    constructor(status: number, message: string) {
+      super(message);
+      this.status = status
+      Object.setPrototypeOf(this, CustomError.prototype)


Very curious why you included this. I have no idea if it's right or wrong or anything; this isn't something I did in my implementation and I'm curious as to why you did this.

ianandrewson · 2021-12-21T00:28:32Z

src/index.ts

+        const user = this.users.find(existingUser => existingUser.id === id)
+
+        if (user) {
+            new UserAPI(this.users.filter(user => user.id !== id))


I still firmly believe there is a way to do this without instantiating a new version of the class.

ianandrewson · 2021-12-21T00:29:23Z

src/index.ts

+                existingUser = { ...updatedUser }
+                return { id, ...existingUser }
+            } else 
+            throw new CustomError(500, "Different user with same id already exists.")


rachoac · 2021-12-21T01:00:27Z

src/index.ts

+        const foundUser = this.users.find(user => user.id === id)
+        if (foundUser === undefined) {
+            throw new CustomError(500, "No user found.")
+        } else return foundUser  


rachoac · 2021-12-21T01:00:49Z

src/index.ts

+
+    public getUserById = (id: number): User => {
+        const foundUser = this.users.find(user => user.id === id)
+        if (foundUser === undefined) {


rachoac · 2021-12-21T01:01:33Z

src/index.ts

+        if (userIdExists) {
+            throw new CustomError(500, "User with id already exists.")
+        } else { 
+            new UserAPI([user, ...this.users])


Why is this returning a new instance of the api

ianandrewson · 2021-12-21T18:57:33Z

src/index.ts

+        const user = this.users.find(existingUser => existingUser.id === id)
+
+        if (user) {
+            this.users = this.users.filter(user => user.id !== id)


rachoac · 2021-12-22T19:53:59Z

src/index.ts

+        if (user) {
+            this.users = this.users.filter(user => user.id !== id)
+            return user
+        } else throw new CustomError(404, "No user with that id found.")


rachoac · 2021-12-22T19:54:27Z

src/index.ts

+        if (targetUser) {
+            this.users = this.users.map(user => {
+                    if (user.id === id) {
+                        if (user.name === updatedUser.name) {


michmitz added 4 commits December 17, 2021 12:19

getUserById and getUsers tests passing

88eabdf

Tests passing for createUser

7d1567e

Update functions to throw error, test errors thrown

1621738

updateUser function, testing edge cases and errors

82aef28

ianandrewson reviewed Dec 21, 2021

View reviewed changes

rachoac requested changes Dec 21, 2021

View reviewed changes

michmitz added 3 commits December 21, 2021 10:11

Update status codes

cd63759

Remove instantiation of new class in functions

08dd7a6

Remove error when users array is empty

ef0d608

ianandrewson reviewed Dec 21, 2021

View reviewed changes

michmitz added 2 commits December 21, 2021 12:11

Fix updateUser function and test

e20000b

Fix updateUsers to only return updated user

142a02e

rachoac reviewed Dec 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step 2 - Implementing Functions #10

Step 2 - Implementing Functions #10

michmitz commented Dec 17, 2021 •

edited

Loading

ianandrewson left a comment

ianandrewson Dec 20, 2021

ianandrewson Dec 20, 2021

ianandrewson Dec 21, 2021

ianandrewson Dec 21, 2021

ianandrewson Dec 21, 2021

ianandrewson Dec 21, 2021

ianandrewson Dec 21, 2021

ianandrewson Dec 21, 2021

ianandrewson Dec 21, 2021

ianandrewson Dec 21, 2021

rachoac Dec 21, 2021

rachoac Dec 21, 2021

rachoac Dec 21, 2021

ianandrewson Dec 21, 2021

rachoac Dec 22, 2021

rachoac Dec 22, 2021

Step 2 - Implementing Functions #10

Are you sure you want to change the base?

Step 2 - Implementing Functions #10

Conversation

michmitz commented Dec 17, 2021 • edited Loading

ianandrewson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michmitz commented Dec 17, 2021 •

edited

Loading