Code review requested: Fruit inventory coding challenge
-
So as part of my effort to make myself marketable for some type of programming position, I've started working on some minor projects to put on my previously barren Github page. First one up: a reimplemenation of @cartman82's most popular interview challenge (with permission), and some solutions in different languages:
(current commit is d18a1ac)
While talking with him about it @cartman82 suggested I post in here if I want any sort of code review, which sounded like a good idea so here I am. Currently there's only a rough C# console app + library solution, which I don't consider finished yet even though it works, and the more important
TestSource/
node+express project, which I think is about done and what I'm (mostly) seeking review and comment on.TestSource is the code behind my version of the challenge presentation site, at https://resonant-shoulder.glitch.me/. In addition to the original sample file from the latest interviews, it has three new "files":
- One that makes a list of random fruit, amounts, and prices but with the same column sizes and ordering,
- One that also varies the column sizes, and
- One that adds column reordering as well.
One obvious problem is no unit testing or whatnot, because I don't know how / what to use for it. Instructions and links on that are welcome too.
-
Spoiler Alert: In the next season of The Mettle there won't be any fruity business... Goat Counting? Lets speculate.
-
@dse said in Code review requested: Fruit inventory coding challenge:
Goat Counting?
-
@ben_lubar said in Code review requested: Fruit inventory coding challenge:
@dse said in Code review requested: Fruit inventory coding challenge:
Goat Counting?
I have a weekly technical metrics feed that goes into some newsletter nobody reads. Totally adding goat teleportation.
-
Ok, here are my critiques for the node/glitch app. I've done the fruits thing so many times, I have little else to say about it. Someone else can handle that.
Clenliness
Cleanup the README.md file from glitch-related stuff. The same goes for boilerplate leftover files inside
public
&views
. If someone is gonna look through this repository, you don't want them to see sloppy leftovers from the template you used. Just your code.Polyfills
Get rid of this polyfill:
You generally don't do polyfills in node, since you already know which version of runtime you'll target. If it has the func you need, there you go. If not, then you just add a standalone function in tools.js or something.
Polyfills are IMO an antipattern and should be avoided whenever possible.
Layering
You generally don't want to let your http-related crap bleed into your business logic. In this case, the only reason
getFruitsTable
needsresponse
is so it can doresponse.send()
;Better pattern is to terminate http at the request handler level and feed processed data into your business logic.
Also, this is a bad error handling practice.
This will crash the server.
Here's a better way to handle both of these:
app.get("/vary-everything.txt", function (request, response, next) { getFruitsTable(variableWidths, variableOrder, (err, table) => { if (err) { return next(err); } response.set({"Content-Type":"text/plain; charset=UTF-8"}); response.send(table); }); }); const getFruitsTable = function(columnSizer, columnOrderer, callback) { fs.readFile('fruits.txt', 'utf8', function(err, fruits) { if (err) return callback(err); // Generate table callback(null, table); }); };
Everything to do with http remains in the request handler.
getFruitsTable
is "pure" in that it can be extracted out of the web app and put into, for example, unit tests (more on that later).Error is handled by returning it to request handler, which then passes it back to express. You'd generally put some global error handler at the end of the express chain, which will then generate a proper response, shoot off an email notification or whatever.
Code structure
This is a tiny project, but IMO you should still show that you have some idea about code layout.
I'd format this into three files:
-
server.js
Your express app with request handlers. The entry point. -
tools.js
Your utility functions (padLeft, shuffle, etc). This should be a module that just exports a bunch of pure functions. -
bl.js
Everything to do with data generation. In this example, these can also be pure functions, which will help with...
Unit testing
In real world, you rarely get to test everything. You pick most important parts and focus automatic testing on those, leaving the thorny non-functional parts for human testers to cover.
In this project, testing priorities are
- Business logic (always a good idea)
- Utility functions (often copy-pasted, so pick and chose what you think is important)
- Express code, http, file access (waste of time in most cases).
Your BL code is already mostly testable, you just might want to move
fs.readFile
out of your main generator function and have it take an already loaded list of fruits.So the final refactor of fruits table code would look like this:
// You don't need to test this, as it's pretty straightforward const loadAndGetFruitsTable = function(columnSizer, columnOrderer, callback) { fs.readFile('fruits.txt', 'utf8', function(err, fruits) { if (err) return callback(err); fruits = fruits.split('\n'); const table = getFruitsTable(fruits, columnSizer, columnOrderer); return callback(null, table); }; const getFruitsTable = function(fruits, columnSizer, columnOrderer) { // Just pure testable code };
Your express code would call
loadAndGetFruitsTable
and your unit tests would deal with justgetFruitsTable
. Bonus benefit,getFruitsTable
is no longer asynchronous, which makes it even easier to test.Conclusion
In general, this is all pretty good coding. I'd recommend to hire you as a junior, on a fast path to becoming a "medior" or whatever you want to call it.
-
-
@dreikin said in Code review requested: Fruit inventory coding challenge:
One obvious problem is no unit testing or whatnot, because I don't know how / what to use for it. Instructions and links on that are welcome too.
Forgot about this. I like
mocha
(a test runner) andchai
(an all powerfulexpect
function, for assertions).Just google it. It's pretty easy to set up unit tests, just follow the examples on their sites.
You don't need functional tests for this project IMO, so don't worry about mocking, or simulating browser requests or any of that crap.
-
@cartman82 In that stack,
nyc
makes it stupid easy to get coverage (way better than the terrible api in theistanbul
package proper, glad they split off the cli stuff). It can be real handy when writing your first unit tests to see which paths you missed.Use
sinon
if you have any external dependencies.
-
@yamikuronue said in Code review requested: Fruit inventory coding challenge:
@cartman82 In that stack, nyc makes it stupid easy to get coverage (way better than the terrible api in the istanbul package proper, glad they split off the cli stuff). It can be real handy when writing your first unit tests to see which paths you missed.
I actually never had the chance to create enough unit test density to even dream of using a code coverage tool. :(
-
@cartman82 I just added
nyc
to my latest utility project. It was two steps:npm install nyc
- in my package.json, change my test target from
mocha test.js
tonyc mocha test.js
Done. Instant coverage results.
(normally I have a test directory, but this is a one-file utility module).
-
@yamikuronue That looks cool: we should maybe switch SockBot to it?
-
@yamikuronue said in Code review requested: Fruit inventory coding challenge:
Done. Instant coverage results.
(normally I have a test directory, but this is a one-file utility module).Oh I am sure I could figure out how to set it up.
I am just not sure I dare see the results :)
-
@raceprouk said in Code review requested: Fruit inventory coding challenge:
we should maybe switch SockBot to it?
It's the same thing we already have under the hood, but they forked out the command-line to a standalone package. I think some of our newer repositories are already using it.
-
@yamikuronue said in Code review requested: Fruit inventory coding challenge:
@raceprouk said in Code review requested: Fruit inventory coding challenge:
we should maybe switch SockBot to it?
It's the same thing we already have under the hood, but they forked out the command-line to a standalone package. I think some of our newer repositories are already using it.
though if sockbot isn't already using it...... PRs accepted.
-
On my way to
St. Ivesimplementing @cartman82's suggested changes I decided to enable eslint in VS Code using the "standard" style guide. It's telling me to get rid of (almost?) all the semicolons. Is that normal for JS?
-
@dreikin It's the preferred style of some more hipsterish javascript coders. Whoever made those eslint rules followed that style.
I personally prefer to have semicolons. But in the end, it's mostly just a personal preference.
-
@cartman82 said in Code review requested: Fruit inventory coding challenge:
It's the preferred style of some more hipsterish javascript coders.
ASI is evil. I refuse to acknowledge it exists.
-
var x = "my return value" return x
-
@jaloopa said in Code review requested: Fruit inventory coding challenge:
var x = "my return value" return x
i see......
I respected you once. and in recognition of that past respect I will offer you a choice. Do you wish to be beaten to within an inch of your life with a small mouth bass or a rainbow trout?
-
@accalia said in Code review requested: Fruit inventory coding challenge:
@cartman82 said in Code review requested: Fruit inventory coding challenge:
It's the preferred style of some more hipsterish javascript coders.
ASI is evil. I refuse to acknowledge it exists.
Oooo you're gonna hate my choice.
-
@dreikin said in Code review requested: Fruit inventory coding challenge:
@accalia said in Code review requested: Fruit inventory coding challenge:
@cartman82 said in Code review requested: Fruit inventory coding challenge:
It's the preferred style of some more hipsterish javascript coders.
ASI is evil. I refuse to acknowledge it exists.
Oooo you're gonna hate my choice.
......
-_-
HIPSTER!
-
@accalia said in Code review requested: Fruit inventory coding challenge:
@dreikin said in Code review requested: Fruit inventory coding challenge:
@accalia said in Code review requested: Fruit inventory coding challenge:
@cartman82 said in Code review requested: Fruit inventory coding challenge:
It's the preferred style of some more hipsterish javascript coders.
ASI is evil. I refuse to acknowledge it exists.
Oooo you're gonna hate my choice.
......
-_-
HIPSTER!
Hehehe. First time I've ever been called that.
My reasoning basically boils down to:
- Following a standard is (probably) better than not, and
- This way I'll be expecting it rather than wondering why my valid looking code isn't.
-
@jaloopa said in Code review requested: Fruit inventory coding challenge:
var x = "my return value" return x
Worst best part: I can see the JS community adopting this enthusiastically.
-
@weng said in Code review requested: Fruit inventory coding challenge:
I can see the JS community adopting this enthusiastically.
Probably not. This is a classic example of why ASI is bad.
It would be translated into something like
var x = "my return value"; return; x;
which also goes to show why it's a bad idea to have such a loose language as Javascript, as in C#, the second one wouldn't compile due to the different return type and the
x
doing nothing
-
@jaloopa Not to mention it wouldn't compile in C# because it doesn't guess where you meant to put semicolons ;)
-
@dreikin said in Code review requested: Fruit inventory coding challenge:
@accalia said in Code review requested: Fruit inventory coding challenge:
@cartman82 said in Code review requested: Fruit inventory coding challenge:
It's the preferred style of some more hipsterish javascript coders.
ASI is evil. I refuse to acknowledge it exists.
Oooo you're gonna hate my choice.
Don't hate the choice, hate the person.
This goes along with my philosophy on stupid questions: There are no stupid questions, just stupid people.
But seriously, I think it's useful for
onclick="doTheClick()"
style event handlers but it's poison for substantial coding. I'd rather have a mouse typer than someone who shunned semicolons in their code.
-
@weng The "tslint" that TypeScript project I worked on recently was using required semicolons.
Who came up with "lint" to mean "advice on formatting code", BTW? "Format your code like the shit that clogs up your dryer."
-
@blakeyrat said in Code review requested: Fruit inventory coding challenge:
Who came up with "lint" to mean "advice on formatting code", BTW?
You love to blame UNIX/Linux, and in this case, you'd be 100% right.
-
@asdf A long continuation of the Unix Philosophy of Naming, where the Create File command is named
touch
, the Documentation command is namedman
(<sjw>WHY IS THERE NO WOMAN COMMAND?!?</sjw>), and the Search command sounds like it's named after one of the less savory bodily functions.
-
@masonwheeler said in Code review requested: Fruit inventory coding challenge:
Search command sounds like it's named after one of the less savory bodily functions.
Find? Locate? Or Grep?
-
@masonwheeler
touch
isn't a tool to create files: its purpose is purely to set the Last Modified time. The fact it creates a file that doesn't exist is a side-effect, which also applies tocat
and a host of other tools.man
is obviously short formanual
, and if you're talking aboutgrep
, then that's 'globally search a regular expression and print'.
-
@dse
grep
, obviously. Is that not exactly what it sounds like? (And even if you didn't think so before, now that it's been pointed out...)
-
@raceprouk said in Code review requested: Fruit inventory coding challenge:
cat
What a perfectly obvious name for the "Print File To Terminal" command!
man
is obviously short formanual
Which online documentation is not. Manuals are paper books that you have to search through manually instead of being able to input exactly what section you're looking for.
and if you're talking about
grep
, then that's 'globally search a regular expression and print'.Because that's so much better an acronym than
gsareap
? What's wrong with simply calling it "search" anyway?The Unix philosphy! As you
touch
so shall yougsareap
!
-
@masonwheeler said in Code review requested: Fruit inventory coding challenge:
@raceprouk said in Code review requested: Fruit inventory coding challenge:
cat
What a perfectly obvious name for the "Print File To Terminal" command!
Except
cat
is for concatenating one or more streams into one stream. The fact it can print to a terminal is simply down to the fact it concatenates the file stream into the standard-out stream.
-
@raceprouk said in Code review requested: Fruit inventory coding challenge:
@masonwheeler
touch
isn't a tool to create files: its purpose is purely to set the Last Modified time. The fact it creates a file that doesn't exist is a side-effect, which also applies tocat
and a host of other tools.Yes, the proper call to create a file is
open
. That makes so much more sense.
-
@masonwheeler but grep isn't a search tool. But I agree, CLI tools on Linux are too stingy with their character count. Any sane shell has Tab completion and reverse search (hopefully bound to arrows). So, do you prefer
Select-String
?
-
@dse What would you call
grep
if not a search tool? Its primary functionality is "search for this text within this set of files/directories," right? Or is this another example of it being intended to do some other obscure thing and everyone uses it for that instead, like @RaceProUK keeps coming up with? (So much for "do one thing"...)
-
@masonwheeler it is a search tool alright, it is just not the search tool. There are
find
andlocate
for that. Also,grep
can process standard input (output of other commands) too
-
@masonwheeler And the command to rename a file is "mv" which stands for move.
Which is a great metaphor, because if something gets renamed in real life (for example, Istanbul to Constantinople), it also changes location. Everybody knows that.
-
@blakeyrat That's nobody's business but the Turks'!
-
@anonymous234 said in Code review requested: Fruit inventory coding challenge:
@raceprouk said in Code review requested: Fruit inventory coding challenge:
@masonwheeler
touch
isn't a tool to create files: its purpose is purely to set the Last Modified time. The fact it creates a file that doesn't exist is a side-effect, which also applies tocat
and a host of other tools.Yes, the proper call to create a file is
open
. That makes so much more sense.What about
creat
?
-
@dse said in Code review requested: Fruit inventory coding challenge:
@anonymous234 said in Code review requested: Fruit inventory coding challenge:
@raceprouk said in Code review requested: Fruit inventory coding challenge:
@masonwheeler
touch
isn't a tool to create files: its purpose is purely to set the Last Modified time. The fact it creates a file that doesn't exist is a side-effect, which also applies tocat
and a host of other tools.Yes, the proper call to create a file is
open
. That makes so much more sense.What about
creat
?@POSIX standard said:
The creat() function is redundant. Its services are also provided by the open() function. It has been included primarily for historical purposes since many existing applications depend on it. It is best considered a part of the C binding rather than a function that should be provided in other languages.
Sorry, open's the good one.
-
@anonymous234 yes I know. But speaking of short and stupid names, one must always shame
creat
-
@blakeyrat said in Code review requested: Fruit inventory coding challenge:
@weng The "tslint" that TypeScript project I worked on recently was using required semicolons.
Who came up with "lint" to mean "advice on formatting code", BTW? "Format your code like the shit that clogs up your dryer."
someone probably answered that but, whatever....
it's actually for "delinter" the tool helps you remove the shit that clogs up your dryer from your code.
not the best name in the world but at least it's semi comprehensible.
unlike
AWK
orGREP
or -shuder- the damn fruitastic names that the nodejs community orgasms over.
-
@accalia said in Code review requested: Fruit inventory coding challenge:
it's actually for "delinter" the tool helps you remove the shit that clogs up your dryer from your code.
The stuff that clogs up my dryer is a physical substance. My code is not at all physical.
This is one of those cases of a programmer having an extremely weird way of looking at the world, then subjecting everybody else to it without first asking, "hey guys, do you ever think of your code problems as dryer lint?"
-
@blakeyrat What about belly button lint?
-
@masonwheeler I don't have any of that in my code, either.
-
@blakeyrat said in Code review requested: Fruit inventory coding challenge:
This is one of those cases of a programmer having an extremely weird way of looking at the world,
i won't argue with that.
still, at least this one is semi understandable once the train of logic is explained.
-
@accalia I dunno. I think there's something wrong with that train of logic. It might even be sick. That's what we should call it: ill logic.
-
@masonwheeler said in Code review requested: Fruit inventory coding challenge:
@accalia I dunno. I think there's something wrong with that train of logic. It might even be sick. That's what we should call it: ill logic.
it might be sick, but it ain't dead yet.
maimed, maybe. Dead, not yet.
-