Week 4 Homework - Super team picker - #4
Conversation
maxwellgordon
left a comment
maxwellgordon
left a comment
There was a problem hiding this comment.
Overall, very good.
A couple of things to take note for next time, but you got all of the important functionality and styling.
Well done with the indentation, too!
| "description": "super team picker", | ||
| "main": "app.js", | ||
| "scripts": { | ||
| "start": "nodemon app.js" |
There was a problem hiding this comment.
in scripts, it is helpful to include scripts for creating/dropping a database and running migrations.
something like
"scripts": {
"start": "nodemon app.js",
"db:create": "createdb --echo super_team_picker",
"db:drop": "dropdb --echo --if-exists super_team_picker",
"db:migrate": "knex migrate:latest",
"db:reset": "npm run db:drop && npm run db:create && npm run db:migrate"
}| exports.up = function(knex) { | ||
| return knex.schema.createTable('cohorts', table=>{ | ||
| table.increments("id"); | ||
| table.text("logoUrl"); |
There was a problem hiding this comment.
dataType text is for long strings, like paragraphs, long lists.
Whereas the string type is for shorter strings (with a maximum length of 255 characters).
It would be more appropriate for this to be a string type.
| return knex.schema.createTable('cohorts', table=>{ | ||
| table.increments("id"); | ||
| table.text("logoUrl"); | ||
| table.text("name"); |
There was a problem hiding this comment.
It would be more appropriate for this to be a string type.
| table.increments("id"); | ||
| table.text("logoUrl"); | ||
| table.text("name"); | ||
| table.string("members"); |
There was a problem hiding this comment.
Since this list could be very long, it would be more appropriate for this to be a text type.
| const baseRouter = require("./routes/base.js") | ||
| app.use('/', baseRouter); | ||
|
|
||
| app.use( |
There was a problem hiding this comment.
It is better for all of your middlewares (like this one) to come before all of your routes. (like the one written on line 10).
Otherwise, if you had a route within the baseRouter that is for a PATCH or a DELETE for some reason, then currently they would NOT work.
| const members = cohort.members; | ||
|
|
||
|
|
||
| function teamBuilder (strOfMembers, quantity, method){ |
There was a problem hiding this comment.
It is unadvised to have this function defined within another function. There are cases where you might want to do this, but in this case it is bad form.
It makes this function massive, and unruly and hard to read/follow.
I liked how you put it in a separate file earlier, that is the best way to do it.
From there, I would use module.exports to export that function from the other file, and then require that function here in order to use it.
That would be the best way to do this.
| } else {break} | ||
| } | ||
| } else { | ||
| break |
There was a problem hiding this comment.
It is unadvised to use break statements within ifs, or for loops.
sometimes you need to, but usually there is a better way of defining your for-loop/termination condition or if-statements so that you don't need to do this.
| <div class="container mt-3"> | ||
| <div class="border p-3"> | ||
| <div class="d-flex align-items-center" > | ||
| <% if (cohort.logoUrl) {%> |
| <h3><%=cohort.name%></h3> | ||
| <p> <%=cohort.members%> </p> | ||
| <hr> | ||
| <form action="/cohorts/<%= cohort.id %>/form" method = "get"> |
There was a problem hiding this comment.
should be a GET to /cohorts/:id, not to this custom route. as defined in the homework.
| @@ -0,0 +1,52 @@ | |||
| //Below is just a reference file for my teamBuilder function | |||
| //This is embedded inside my cohorts.js file | |||
| function teamBuilder (strOfMembers, quantity, method){ | |||
There was a problem hiding this comment.
Should use this like a utility or library function.
Recommended to place this file within a lib or a utils folder`.
Then, at the bottom of the file, write:
module.exports = teamBuilder;And within another file, you can require this function from this file:
const teamBuilder = require('../path/to/this/file/utils/teamBuilder');And use the function there.
2 participants
Hi Max,
Please find attached my pull request for my week 4 home - Super Team Picker. I am having issues with git, so you'll see my week 3 folder there. I'll try and fix that.
Thanks,
David