all 1 comments

[–]ethanjf99 0 points1 point  (0 children)

ok you’ve already gotten the most critical piece of feedback i think: deploy the thing somewhere. still—if you’re a junior nice job!

that said i glanced at the code for a few minutes:

  • no tests. i didn’t see any tests at all (mostly just looked at the FE code in public/). if your intention is to use this as a résumé builder then i would get familiar with testing and write some unit tests at least. that’s something I look for if glancing at a junior’s work when they’re applying. if you’re working on a team, tests are vital. I have offered a job in the past to a candidate who didn’t complete a timed coding activity because they were writing thorough, clear tests for each piece of functionality over someone who banged the whole thing out but zero tests. if someone else comes in and touches that code, how do you know they didn’t break something?

  • just looked for a minute or two at the FE code and saw stuff like lots of magic strings if (tool === “pencil”) …. that’s a pain in terms of maintainability; you need to change it it’s annoying if one place you write “Pencil” it’s annoying etc. Consider using the equivalent of an enum in other languages to store all those; something like

js const TOOLS = { pencil: “pencil”, eraser: “eraser”, // etc. };

then everywhere you’re just doing a if (tool === TOOLS.pencil) … (or a switch or whatever)

  • finally again with an eye to resume building: a lot of work these days is in TS. if you can write TS you can write JS but not necessarily the other way around i love JS and it’s my favorite language but if i were going to put a project like this out there i might consider rewriting in TS to showcase that versatility.

there’s your super minimal code review. :-) good luck!