Skip to content

Conversation

AlexanderLH
Copy link

@AlexanderLH AlexanderLH commented Aug 18, 2020

Copy link
Collaborator

@vramaniuk vramaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll mark the PR as "Draft", please click "ready for review" when it will be finished. Thank you!

Comment on lines 117 to 122
let result = '';

for (let i = 0; i < count; i++) {
result += value;
}
return result;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/
function getDistanceBetweenPoints(x1, y1, x2, y2) {
throw new Error('Not implemented');
return Math.sqrt(Math.pow(x2 - x1, 2) + Math.pow(y2 - y1, 2));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/
function getLinearEquationRoot(a, b) {
throw new Error('Not implemented');
return (b === 0) ? 0 : -b / a;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we don't need to check b === 0, because -b / a equals zero if (b ===0)

Copy link
Author

@AlexanderLH AlexanderLH Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we don't need to check b === 0, because -b / a equals zero if (b ===0)
Hello. If i remove this check (b === 0), the result of the function will be -0 (minus zero). In this case the test with parameters a=5 and b = 0 fails.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we don't need to check b === 0, because -b / a equals zero if (b ===0)
Hello. If i remove this check (b === 0), the result of the function will be -0 (minus zero). In this case the test with parameters a=5 and b = 0 fails.

But it shouldn't fail, because as we can see on screenshot (please, try it in your chrome console 0 === -0):
Selection_036

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but for some reason this test fails and I don't know why
getLinearEquationRoot

Copy link
Collaborator

@vramaniuk vramaniuk Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we have successful travis build for such solution here for example:
#928

https://travis-ci.org/github/AleshaKolesnikov/js-assignments/builds/719550067
Please, try to find differences.
May be node versions, travis.yml

Comment on lines 115 to 116
const vectorProductX = Math.sqrt((x1 * x1) + (y1 * y1));
const vectorProductY = Math.sqrt((x2 * x2) + (y2 * y2));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use Math.hypot here

Comment on lines 94 to 98
const formatToThreeDigit = (number) => number < 10
? `00${number}`
: number > 10 && number < 100
? `0${number}`
: number;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please,avoid this code style, if-else is more readable than multiple conditional operators ? :

*/
function getItemsSum(arr) {
throw new Error('Not implemented');
return (arr.length) ? arr.reduce((sum, currentItem) => sum += currentItem) : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant logic, we can use just reduce with initial value 0
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce

*/
function getFalsyValuesCount(arr) {
throw new Error('Not implemented');
return (arr.length) ? arr.reduce((sum, currentItem) => (currentItem) ? sum += 0 : sum += 1, 0) : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rework this line without ternary operators.

*/
function findAllOccurences(arr, item) {
throw new Error('Not implemented');
return arr.reduce((counter, currentItem) => (currentItem === item) ? counter += 1 : counter, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rework this line without ternary operators.

Comment on lines 418 to 440
throw new Error('Not implemented');
return arr.sort((a, b) => (a.country > b.country) ? 1 :
(a.country < b.country) ? -1 :
(a.country === b.country) ? (a.city > b.city) ? 1 :
(a.city < b.city) ? -1 : 0 : 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, avoid this code style, if-else is more readable than multiple conditional operators ? :

@vramaniuk vramaniuk marked this pull request as draft August 19, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants