Eric Whitcomb #481
Eric Whitcomb #481
Conversation
|
Link to frontend project pull request- Link to frontend deployed on netlify- Link to backend deployed on heroku- |
|
Should be done with MVP and on to stretch goals |
|
|
||
| const get = async (id) => { | ||
| if (id) { | ||
| const note = await db('notes').where('id', id); |
There was a problem hiding this comment.
I'm pretty sure you can use .first()? instead of having that if statement
| const note = await db('notes').where('id', id); | |
| const note = await db('notes').where('id', id).first(); |
| if (note.length) { | ||
| return note[0]; | ||
| } else { | ||
| const e = new Error("id does not exist"); |
There was a problem hiding this comment.
Might be easiest just to do this all on one line
| const e = new Error("id does not exist"); | |
| throw new Error("id does not exist"); |
There was a problem hiding this comment.
Then you won't need the next two lines
| const insert = async (note) => { | ||
|
|
||
| // check for missing note object | ||
| if (typeof note === 'undefined') { |
There was a problem hiding this comment.
I don't think you need this check 🤔 is there ever a case where this would happen?
| } | ||
|
|
||
| // check if note is object | ||
| if (typeof note !== 'object') { |
There was a problem hiding this comment.
Same for this. Would this ever not be an object?
| } | ||
|
|
||
| // check for missing title key | ||
| if (!note.hasOwnProperty('title')) { |
There was a problem hiding this comment.
Since note isn't inherited, you are fine with just checking !note.title
| if (!note.hasOwnProperty('title')) { | |
| if (!note.title) { |
Side Note: Now if it something like
const originalNote = { title: 'Here is a title', content: 'Some content' }
const note = originalNote;
console.log(note.hasOwnProperty('title')) // false because it is inherited
note.tag = 'here is a tag'
console.log(note.hasOwnProperty('tag)) // true
| } | ||
|
|
||
| // check for title not string | ||
| if (typeof note.title !== 'string') { |
There was a problem hiding this comment.
I think this will always be a string. Even if the user types in a number like 1 it will still be a string unless you change it to a number lol
| } | ||
|
|
||
| // check for content not string | ||
| if (typeof note.content !== 'string') { |
| } | ||
|
|
||
| // check for missing content key | ||
| if (!note.hasOwnProperty('content')) { |
There was a problem hiding this comment.
| if (!note.hasOwnProperty('content')) { | |
| if (!note.content) { |
| return n; | ||
| }; | ||
|
|
||
| const update = async (id, note) => { |
There was a problem hiding this comment.
Same thing as above. Double check to make sure you aren't making unnecessary checks
| throw e; | ||
| } | ||
|
|
||
| const count = await db('notes').where('id', id).update(note); |
There was a problem hiding this comment.
This is a good time to have a `try/catch block to organize your code
try {
await db('notes').where('id', id).update(note); // <-- if this fails it will automatically go to the catch block
const n = await get(id);
return n;
} catch (e) {
throw new Error("id does not exist");
}
| test("throws MissingKey when 'title' key not present in note object", async () => { | ||
| try { | ||
| await noteModel.insert({ content: "Content" }); | ||
| expect(true).toBe(false); |
There was a problem hiding this comment.
Lol you don't have to have this here I don't think
| expect(true).toBe(false); |
| test("throws MissingKey when 'content' key not present in note object", async () => { | ||
| try { | ||
| await noteModel.insert({ title: "Title" }); | ||
| expect(true).toBe(false); |
There was a problem hiding this comment.
Same here
| expect(true).toBe(false); |
| test("throws TypeError when 'title' value is not string", async () => { | ||
| try { | ||
| await noteModel.insert({ title: 0, content: "Content" }); | ||
| expect(true).toBe(false); |
There was a problem hiding this comment.
| expect(true).toBe(false); |
| test("throws TypeError when 'content' value is not string", async () => { | ||
| try { | ||
| await noteModel.insert({ title: "Title", content: 0 }); | ||
| expect(true).toBe(false); |
There was a problem hiding this comment.
| expect(true).toBe(false); |
@KingAtoki