vasyabester / epam_fe_2019_tyurin

Apache License 2.0
0 stars 0 forks source link

HW12 #12

Open YuliyaAstakhova opened 4 years ago

YuliyaAstakhova commented 4 years ago

(value) => {...} === value => {...} We don't need parentheses here, because arrow function got only one value.

Task #2 You have a lot of redundant code, try to find another way to calculate. For example, you don't need calculate firstString and secondString, you can use function arguments (first, second) instead, you can use inverse cycle:

const lengthOfLoops = Math.max(first.length, second.length) + 1;

for (let i = lengthOfLoops; i >= 0; i--) {
    const firstDigit = +first[i] || 0;
    const secondDigit = +second[i] || 0;
    .......
}

Task #3 You understand task incorrectly. If there aren’t post or comments, should return string: 'posts: 0, comments: 0'

vasyabester commented 4 years ago

Hello! Yuliya! Sorry that I didnt answer on previous feedbacks. What about (value) => {...} === value => {...}, I know that I dont need to put parentheses here, but Lintr said that it was error, so I had to fix it. What about task 3 - of course its logically if you dont find any post and comments of the specific author - the result should be 'posts: 0, comments: 0', but in home task it was like in screenshot - even bold font and I did my task due to requirements. Thanks for checking.

пт, 6 дек. 2019 г. в 00:18, Yuliya Astakhova notifications@github.com:

(value) => {...} === value => {...} We don't need parentheses here, because arrow function got only one value.

Task #3 https://github.com/vasyabester/epam_fe_2019_tyurin/issues/3 You understand task incorrectly. If there aren’t post or comments, should return string: 'posts: 0, comments: 0'

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vasyabester/epam_fe_2019_tyurin/issues/12?email_source=notifications&email_token=ANOUJ3KP3VDLHA36DWOMABTQXF43RA5CNFSM4JWDAEJKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H6OSDUQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANOUJ3PVFVGJCSAHAN6PYCTQXF43RANCNFSM4JWDAEJA .

YuliyaAstakhova commented 4 years ago

Hello! Added one more comment..... feedback for task#1 will come soon )) Next time If you will doubt about requirements, ask me! you can contact me at skype: july_belokurova

YuliyaAstakhova commented 4 years ago

Task #1 Use more formal names. Actually by the task your main function should be named "tickets()". https://github.com/vasyabester/epam_fe_2019_tyurin/blob/bafe8cd15b94e10d4885b4462e5239a214dec3b6/HW12_Functions/Task1/js/main.js#L2 You don't need this constant, use visitorsBanknoteList instead.

The one main problem is that your realization is not scalable. The logics must be independent from input parameters. Imagine that we add one more type of bill or apply this task to real life with a lot of different coins and bills. What will you do? How many functions like "giveChangeFor50" will you add more? :) Try make all implementations is abstract, and configure it dynamically according to input data.

For example, you can define constant with an array of all available types of bill. It allows you to add one more type or remove it any time. const banknotes = [ 25, 50, 100 ];

Also you can keep information about current banknotes in properties of object: vasyasBanknotes = banknotes.reduce( ( result, type ) => { [ type ] : 0 } ); It allows not to look for an appropriate banknote in one array every time and reduce calculations.

Other calculations can be reworked by similar way

vasyabester commented 4 years ago

Thanks for feedback Yuliya. I know that my realization was not the best, but I didnt find shortest way, so I decided to do variant which is working, but not with the simple decision))) What about naming of function, I dont know if it recorded on video lection, but Oleg told us that its not the best name of function tickets() and strongly recommended to us rename it. Thanks for checking.

пт, 6 дек. 2019 г. в 01:10, Yuliya Astakhova notifications@github.com:

Task #1 https://github.com/vasyabester/epam_fe_2019_tyurin/issues/1 Use more formal names. Actually by the task your main function should be named "tickets()".

https://github.com/vasyabester/epam_fe_2019_tyurin/blob/bafe8cd15b94e10d4885b4462e5239a214dec3b6/HW12_Functions/Task1/js/main.js#L2 You don't need this constant, use visitorsBanknoteList instead.

The one main problem is that your realization is not scalable. The logics must be independent from input parameters. Imagine that we add one more type of bill or apply this task to real life with a lot of different coins and bills. What will you do? How many functions like "giveChangeFor50" will you add more? :) Try make all implementations is abstract, and configure it dynamically according to input data.

For example, you can define constant with an array of all available types of bill. It allows you to add one more type or remove it any time. const banknotes = [ 25, 50, 100 ];

Also you can keep information about current banknotes in properties of object: vasyasBanknotes = banknotes.reduce( ( result, type ) => { [ type ] : 0 } ); It allows not to look for an appropriate banknote in one array every time and reduce calculations.

Other calculations can be reworked by similar way

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vasyabester/epam_fe_2019_tyurin/issues/12?email_source=notifications&email_token=ANOUJ3PX4TEBWLIFL7FEAPDQXGDAHA5CNFSM4JWDAEJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGCO33I#issuecomment-562359789, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANOUJ3PYTQURNL3P7276HGDQXGDAHANCNFSM4JWDAEJA .