Review (not for merge) - #19
Conversation
Develop processing v0.0.1
Create CODE_OF_CONDUCT.md
|
@tshemsedinov Good afternoon, can you please do us a code review when you have some free time? |
| @@ -0,0 +1,3 @@ | |||
| node_modules/ | |||
| static/ | |||
| test/ No newline at end of file | |||
|
|
||
| const path = require('path'); | ||
|
|
||
| const cachingRequire = (cacheSize = 10) => { |
There was a problem hiding this comment.
10 is a magic number, use CAMEL_UPPER constant
There was a problem hiding this comment.
Fixed it, thanks a lot
| const { exportRes, workerId, error } = message; | ||
| finished++; | ||
|
|
||
| if (error) reject(error); |
There was a problem hiding this comment.
Maybe reject and return
There was a problem hiding this comment.
Fixed it, thanks a lot
| 'use strict'; | ||
|
|
||
| module.exports = (data) => { | ||
| const grayscaleCofs = [0.2126, 0.7152, 0.0722]; |
| for (let i = 0; i < length; i += 4) { | ||
| const pictureColors = data.slice(i, i + 3); | ||
|
|
||
| const val = pictureColors.reduce( | ||
| (acc, color, index) => acc + color * grayscaleCofs[index], | ||
| 0 | ||
| ); | ||
|
|
||
| data[i] = data[i + 1] = data[i + 2] = val; |
| for (let i = 0; i < data.length; i += 3) { | ||
| data[i] += 40; | ||
| data[i + 1] += 40; | ||
| data[i + 2] += 40; | ||
| } |
There was a problem hiding this comment.
It was originally planned as functions for a sample project, and in the finished project the user could add his own processing algorithms. But I completely agree with you, even though the example looks terrible, we will definitely fix it
| }); | ||
| }); | ||
|
|
||
| const getArgs = (req) => |
There was a problem hiding this comment.
|
@tshemsedinov, we have fixed everything. |
| @@ -0,0 +1,2 @@ | |||
| README.md | |||
| test/ No newline at end of file | |||
There was a problem hiding this comment.
Add \n at EOF all text files
| }; | ||
|
|
||
| const killer = () => { | ||
| workers.forEach((worker) => worker.kill('SIGTERM')); |
There was a problem hiding this comment.
for..of loop will be better here
| for (let i = 0; i < data.length; i++) { | ||
| data[i] += ajustment; | ||
| } | ||
| return data; |
There was a problem hiding this comment.
If you mutate argument, do not need to return it
| } | ||
|
|
||
| async function processImage(req, method) { | ||
| const args = await getArgs(req); |
There was a problem hiding this comment.
Need a try block around getArgs call or around processImage call or upper, errors will pop up, but somewhere you need to catch them.
There was a problem hiding this comment.
We already have it.
https://github.com/Scopics/image-belabour/blob/main/server.js#L27-L34
| fetch(url, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| }) | ||
| .then((response) => { | ||
| return response.json(); | ||
| }) | ||
| .then((data) => { | ||
| imageDataToImg(data); | ||
| }) | ||
| .catch(() => { | ||
| errorBlockElement.innerHTML = 'Error happened'; | ||
| }); |
There was a problem hiding this comment.
Maybe async/await syntax will be cleaner
Co-authored-by: Timur Shemsedinov <timur.shemsedinov@gmail.com>
Co-authored-by: Timur Shemsedinov <timur.shemsedinov@gmail.com>
Co-authored-by: Timur Shemsedinov <timur.shemsedinov@gmail.com>
|
@tshemsedinov, we're all done, thanks a lot |
Co-authored-by: Timur Shemsedinov <timur.shemsedinov@gmail.com>
|
@tshemsedinov we've changed it. Thanks for review. |
3 participants
No description provided.