Conversation
04 bitmap/reader.js
04 bitmap/transformer.js
04 bitmap/writer.js
04 bitmap/process.argv
…ions in lib files
Bessie/testing
finished reader.test.js, broke writer.test.js
04 bitmap/transformer.js grayscale
fixed writer.test.js kinda, and refactored some stuff
to get this branch caught up
trying to figure out how to do writer test
04 bitmap/testing
| }); | ||
| describe('#blackout', () => { | ||
| it('should return a blackout bitmap buffer', (done) => { | ||
| fs.readFile(`${__dirname}/test-data/palette-bitmap.bmp`, function(err, data){ |
There was a problem hiding this comment.
Testing this lab is hard, and this "double read" technique is definitely a clever approach.
| @@ -0,0 +1,22 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Since this file doesn't actually do anything, it should be removed. Or...better yet, test that your writer works by writing the file, and then using fs to ensure that the written file is actually there.
|
|
||
| // node index.js './data/palette-bitmap.bmp' './data/newpalettebitmap.bmp' 'blackout' | ||
| reader(process.argv[2], function (err, data) { | ||
| let buff = transformer(data, process.argv[4]); |
There was a problem hiding this comment.
Consider that your transform function could potentially throw, so it might be worth wrapping the call in a try/catch and handling the errors more elegantly, rather than just blasting the console with the error and letting the app crash.
| "author": "", | ||
| "license": "ISC", | ||
| "dependencies": { | ||
| "jest": "^22.3.0" |
There was a problem hiding this comment.
Don't forget to add eslint as a dependency to every project!
| if(!transformTypes.includes(transformType)) throw new Error('transform type not supported'); | ||
|
|
||
| switch(transformType){ | ||
| case 'blackout': |
There was a problem hiding this comment.
case switches definitely have their place at times, but don't want to be too default in your toolbox -- they are associated with junior devs. Great job getting the transforms to work, though, this is one of the most infamous labs of 401
| var transformTypes = ['blackout', 'invert', 'grayscale', 'invcolors']; | ||
| if(!transformTypes.includes(transformType)) throw new Error('transform type not supported'); | ||
|
|
||
| switch(transformType){ |
There was a problem hiding this comment.
You have a lot of repeated actions in this switch statement. For example, finding the start and offset. It would be much cleaner if you had a function that would do that for you and return the values that you need. Even nicer, if each transform were its own function, your switch case would be lean and clean. User wants this transform, switch case jumps there and calls that function.
| @@ -0,0 +1,79 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Lots of magic numbers in here. They obviously have some significance with respect to the buffer, but that significance is unknown when looking at the code. You should store those in descriptive variable names so someone looking at your code can look and understand what they stand for.
5 participants
No description provided.