Skip to content

Latest commit

 

History

History
310 lines (292 loc) · 20 KB

live-code-reviews.md

File metadata and controls

310 lines (292 loc) · 20 KB

week 1 - Server

Week 2 - Database

Week 3 - Authentication

Week 4

  • TechYEScracy

  • Be weary of redundant lines of code and unnecessary logic
	router.post("/vote", (req, res) => {
	const user = req.signedCookies ? req.signdCookies.user : false;
	// user is never used 
	// that line could also have been: 
	// const user = req.signedCookies && req.%% signedCokies %%.user

	
	const {poll_id,vote_type} = req.query;
	updatePoll(poll_id, vote_type , 1);
	return res.redirect("../")

});

Here's a good chance for a ternary statement:

function getPollList(expired) {
if (expired) return select_all_expiredPolls.all();
return select_all_activePolls.all();

// expired ? select_all_expiredPolls.all() : select_all_activePolls.all()
}
  • Your testing library is a nice set up. Pretty easy to read and the suite you created is fairly comprehensive - barring maybe the description of the actual suite:
test("test test", () => { // changing just this would make your logs clearer
assert.equal(1, "1", "Expected 1 to be equal to '1'");
});

test("can create a new user", () => {
reset();

const before = count("users");
createUser("Egg", 20);

const after = count("users");
assert.equal(
	before,
	after - 1,
	"Expected the count of users before to be one less than after creating a new user"
);

});
  • Version control
    • this repo employs a dev-branch but as a consequence the main branch is actually behind by a few features (even though the dev-branch version is stable) - probably a good idea to review workflows

Comic-unity

  • Consider naming best-practices
    • for example, drawImage() suggests that the function would allow you to draw an image but it's actually returning an HTML template
  • Clean up!!
    • If code is commented out by the time you are ready to push then it needs to be removed
		// function layout({ title, content }) {
// return /*html*/ `
// <!doctype html>
// <html lang="en">
// <head>
// <meta charset="UTF-8">
// <meta name="viewport" content="width=device-width, initial-scale=1.0">
// <link rel="stylesheet" href="normalize.css">
// <link rel="stylesheet" href="styles.css">
// <title>${title}</title>
// </head>
// <body>
// ${content}
// </div>
// </body>
// </html>
// `;
// }

  

// module.exports = layout;
  • Here's a nifty change that would have made your code more iterable:
const colours = ['black', 'white', 'red', 'orange', 'yellow']

<div class="color-controls">
	colours.map(colour => return {`
		<button id='${colour}-button' class='color-button'></button>
	`})

that would get rid of this wall of code:

<div class="color-controls">
	<button id='black-button' class="color-button"></button>
	<button id='white-button' class="color-button white"></button>
	<button id='red-button' class="color-button red"></button>
	<button id='orange-button' class="color-button orange"></button>
	<button id='yellow-button' class="color-button yellow"></button>
	<button id='green-button' class="color-button green"></button>
	<button id='blue-button' class="color-button blue"></button>
	<button id='purple-button' class="color-button purple"></button>
</div>

and it would allow you to expand the palette available in future by adjusting a simple array.

ai-art-gallery

  • I like your README.md
    • clear and detailed - maybe make it a little more visual (e.g. database schema)
  • Nice one, using the github secrets to make your variables available without exposing them
  • just a quick props on this here:
// in routes/gallery.js
router.get("/", async (req, res) => {

	try {
		const artworkDetails = await selectWork();
		const ids = artworkDetails.map(image => image.id);
		const artworkWithImages = await Promise.all(ids.map(async id => {
		const imageDetails = await displayWorks(id);

		return {
		...artworkDetails.find(artwork => artwork.id === id),
		image_file: imageDetails.image_file,
		};
	}));

return res.send(layout(artworkWithImages));
}
- Using the spread operator here is really slick and your logic is akin to what you'll need when you start managing states soon

Week 5

Week5-James_Deepa

PetProgrammer

elena-tess-w5

react-minesweeper-dylan-george

  • lots of variables defined but not used here
  • no instructions for how to run the project in readme
  • you have a few gameState states, they should live in constants so that you reduce the chance of making mistakes and so you can easily know what all the possible states are
  • there's no need for a react fragment here
  • good candidate for a switch statement (but more of stylistic preference rather than anything else)
  • you have both and index.css and an App.css and one of them is empty

sound-control

  • because your state is quite complex, i would consider using useReducer rather than useState
  • compareArrays could have better naming, you're checking for equality rather than just comparing so something along those lines
  • nested ifs and else could look cleaner by employing early return
  • play button and reset button are the same, do they need to be separate components?
  • do they even need to be abstracted out from App.jsx?
  • here you can use && instead of using ternary operator

battleship

  • no instructions for how to run app in readme
  • eslint not set up properly
  • lots of console.logs left in
  • the surrounding div here could just be a fragment
  • this should live in a useEffect, (come and ask me if it doesn't make sense why)
  • is there any advantage to abstracting this button to a different file?
  • there's an empty index.js file which you should remove from the codebase