Logan-Luis-Nick-Bitmap-Final#14
Logan-Luis-Nick-Bitmap-Final#14loganabsher wants to merge 29 commits intocodefellows-javascript-401d17:masterfrom
Conversation
Logan friday
writes the rotated image
Logan monday
added new test for constructor, cleaned up code
| expect(err).to.equal(null); | ||
| fileReader.writeNew(`${__dirname}/../assets/palette-write-bitmap.bmp`, data); | ||
| expect(data).to.not.equal(null); | ||
| fileReader.eraseFile(`${__dirname}/../assets/palette-write-bitmap.bmp`); |
There was a problem hiding this comment.
You are calling this as cleanup but you aren't proving that this works or anything. In fact I would expect this to be the last thing but the file is still there when the tests are done. You should structure your callbacks to ensure that they don't move on at the wrong time.
| fileReader.eraseFile(`${__dirname}/../assets/palette-invert-bitmap.bmp`); | ||
| index.invertBitmap(); | ||
| fileReader.initFile(`${__dirname}/../assets/palette-invert-bitmap.bmp`, (err, data) => { | ||
| expect(err).to.equal(null); |
There was a problem hiding this comment.
Instead of having all the invert bitmap do all the work and then re-reading the file to check if it is good you should structure your code a bit differently so that you can get the results of what you just wrote so you can easily test it without reading the file again. Right now you aren't proving that you aren't reading an empty string. 'I don't think you are but you want to show that you are.'
| }); | ||
| }; | ||
|
|
||
| ColorTransform.prototype.rotateImage = function(bitmap) { |
There was a problem hiding this comment.
This will handle 180 transforms so a good name for it would be flip rather than rotate.
| }); | ||
| }; | ||
|
|
||
| ColorTransform.prototype.blueShift = function (bitmap) { |
There was a problem hiding this comment.
This seems to be turning the bmp very red.
| let g = parseInt(hexArray[1], 16); | ||
| let b = parseInt(hexArray[2], 16); | ||
| let invertR = (Math.ceil(r * 0.1)).toString(16); | ||
| let invertG = (Math.ceil(g * 0.1)).toString(16); |
There was a problem hiding this comment.
This isn't really an invert. A better var name would be shiftR.
| @@ -0,0 +1,20 @@ | |||
| 'use strict'; | |||
| }; | ||
|
|
||
| exports.eraseFile = (path) => { | ||
| return fs.writeFile(path, '', (err, data) => { |
There was a problem hiding this comment.
While this does change the contents of the file there is a better command from fs.unlink that would have erased the file entirely. Leaving it as an empty string can make it seem like there is still a relevant file.
| module.exports = exports = {}; | ||
|
|
||
| exports.invertBitmap = () => { | ||
| let onRead = (err, data) => { |
There was a problem hiding this comment.
This seems like an unnecessary wrapper. Would be more straight forward to have the file helper at top and then could define function. That would flow easier for later reading.
|
|
||
| module.exports = exports = {}; | ||
|
|
||
| exports.invertBitmap = () => { |
There was a problem hiding this comment.
Not taking in a path from anywhere on this means that you only have access to one bitmap file and can't test multiple. It would be good to keep this modular in case you want to read other bitmap files.
4 participants
No description provided.