Friends app by SamVal007 · Pull Request #457 · kottans/frontend-2021-homeworks

SamVal007 · 2021-02-17T18:47:59Z

#Friends_APP

Please, review.

zonzujiro · 2021-02-19T11:48:54Z

submissions/SamVal007/Friends_APP/script.js

+    .then(function (data) {
+      persons = data.results;
+      displayList = persons;
+      displayCards();


Single responsibility - your functions should do only one job. As an example function in which you fetch users should only fetch them and not render, transform or process them in other ways. All these things should be done in another place, outside your function. The same applied to the sort function, which usually does all types of sorting 😉

I would be honest, but i didnt find the way how to delete this displayCards() from this piece of code, so i just renamed function. I was trying to put this displayCards() in document.addEventListener('DOMContentLoaded', function () , but it didnt work...

Tip - you should use then in special place ;)

Yep, this I understood, but where:)
Ok, I will try to figure out!

"in special place" 😄

Ok, I think I found out how to do it!
First of all - I tried to use one more promise inside of document.addEventListener('DOMContentLoaded', function (){...} , but it didnt work, i dont know why. Then I decided to use async and seems it worked!

zonzujiro · 2021-02-19T11:49:38Z

submissions/SamVal007/Friends_APP/script.js

+          displayList = displayList.sort((a, b) => a.dob.age - b.dob.age);
+          break;
+        case 'ageHigh':
+          displayList = displayList.sort((b, a) => a.dob.age - b.dob.age);


Why you need assigning to displayList?

Same function as at line 81. DRY

Also, please check this comment #449 (comment)

Thank you!
Yes, really, i dont need this 'assigning '!
Ok, I will read this comments.

I'm sorry, Did I understand correctly - I should remake this part of code (this switch is too long and it's not "Single responsibility"):

if (radio) {
switch (radio.id) {
case 'genderAll':
displayList = persons;
break;
case 'genderMale':
displayList = persons.filter(el => el.gender === 'male');
break;
case 'genderFemale':
displayList = persons.filter(el => el.gender === 'female');
break;
case 'nameAsc':
displayList.sort((b, a) => a.name.last > b.name.last ? -1 : 1);
break;
case 'nameDesc':
displayList.sort((b, a) => a.name.last < b.name.last ? -1 : 1);
break;
case 'ageLow':
displayList.sort((a, b) => a.dob.age - b.dob.age);
break;
case 'ageHigh':
displayList.sort((b, a) => a.dob.age - b.dob.age);
break;
}

With adding functions for sorting.
BTW, Should I delete "long switch" and make few different "addEventListneres" for each case of switch or it will be very ig load on site? (sorry may be for stupid questions, but i just want to understand)

Yes, you should delete the long switch

You can have a listener on the form and listen to change event. Or you can separate listeners between fieldsets. Up to you

Huh! Unbelievable, but seems I did it!:)
I dodnt know why, but this refactoring took me quit long time.
Basically, I think. It because I start fixing everything at one time... it was big mistake - nothing work and could understand why. So, then I started do it step-by-step, like You teaching us:) Refactored sort logic, then long switch.
Sorry, may be It's not necessary to write it here, but whom else can i write it;))

Ok, so now code looks better, I hope:)

OleksiyRudenko · 2021-02-26T06:49:38Z

@SamVal007 if you think your changes address the reviewer's comments please re-request review

SamVal007 · 2021-02-27T22:14:06Z

I'm sorry that I'm out of deadline.
If you would be so kind, re-review it again, please .

- Add functions for sorting (more structural logic for sorting) - Changed long switch to more compact logic (with using addEventListener('change', ({target: radioButton}))

submissions/SamVal007/Friends_APP/script.js

zonzujiro · 2021-03-02T08:25:46Z

submissions/SamVal007/Friends_APP/script.js

+
+document.addEventListener('DOMContentLoaded', function () {
+  makeFriendList();
+  //displayCards(); //I dodnt know why, but if put displayCards() here -> it doesnt work


So you need to found out :D Or you need my help?

It seems, now it works as we want with single responsibility!
But I watched one video about pure functions and understood, that this my code is smelling a little)) Right?:)
I should make it without using GLOBAL arrays let persons = []; let displayList = [];. Right?

submissions/SamVal007/Friends_APP/script.js

zonzujiro · 2021-03-02T08:51:14Z

submissions/SamVal007/Friends_APP/script.js

+    displayCards();
+  })
+
+  btn.addEventListener('click', () =>{


Dnt us shrtcts n nms :)

Didnt get, what?:)
"Dont use short carts for names"? Right?

Now you understand

Ok, Thanks!:)

@zonzujiro
What do you think about other places? Is there everything good?

zonzujiro · 2021-03-02T08:51:32Z

submissions/SamVal007/Friends_APP/script.js

+
+let persons = [];
+let displayList = [];
+const putPersonsInDisplayList = () => {displayList = persons;};


Function redundant, IMO

submissions/SamVal007/Friends_APP/script.js

Made fixes by review.

zonzujiro · 2021-04-11T12:50:27Z

Sorry, man. I'm with covid right now.

I will return to PR in a week or so.

If not - pls ping me in Telegram :)

SamVal007 · 2021-04-11T13:25:34Z

OMG, I'm sorry, I didn't know. Wish You become Healthy as fast as it is possible. PS. No worries about PR! It doesn't matter right now!

…

On Sun, Apr 11, 2021 at 3:50 PM Ivan Tytarenko ***@***.***> wrote: Sorry, man. I'm with covid right now. I will return to PR in a week or so. If not - pls ping me in Telegram :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#457 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASDPGHGSXMY6P5L6HXZ56TDTIGLJ7ANCNFSM4XY4JNTQ> .

-- С ув. Самойленко В.

zonzujiro

You should pay more attention to code style :) Redundant empty lines, missing curly brackets, etc.

zonzujiro · 2021-04-23T08:12:36Z

submissions/SamVal007/Friends_APP/script.js

+  if (value === 'genderAll') putPersonsInDisplayList()
+  else{
+  displayList = persons.filter(el => el.gender === value);
+  }
+


Broken syntax. Avoid such mistakes :)

Add brackets and use prettier

zonzujiro · 2021-04-23T08:12:53Z

submissions/SamVal007/Friends_APP/script.js

+  makeFriendList()
+    .then(() => {
+      displayCards();   
+    });


Suggested change

makeFriendList()

.then(() => {

displayCards();

});

makeFriendList().then(displayCards);

zonzujiro · 2021-04-23T08:13:35Z

submissions/SamVal007/Friends_APP/script.js

+    });
+
+  SEARCH_FIELD.addEventListener('input', function () {
+    let searchStr = '';


No need in Str in variable. Type of data is redundant info in variable name. Please, find another one.

zonzujiro · 2021-04-23T08:19:55Z

submissions/SamVal007/Friends_APP/script.js

+  MENU_FORMS.addEventListener('change', ({target: radioButton}) => {
+    putPersonsInDisplayList();
+
+    if (radioButton.name === 'genderField') filtrByGender(radioButton.value);


Sometimes you use brackets, sometimes don't.

You should pick one approach and use it :)

zonzujiro · 2021-04-23T08:20:37Z

submissions/SamVal007/Friends_APP/script.js

+    if (radioButton.name === "nameField") {
+      nameSorters[radioButton.value]();
+    }
+
+    if (radioButton.name === "ageField") {
+      ageSorters[radioButton.value]();
+    }


ageSorters[radioButton.name]()?

zonzujiro · 2021-04-23T08:21:25Z

submissions/SamVal007/Friends_APP/script.js

+
+const ageSorters = {
+  ageLow: () => persons.sort((a, b) => compareAge(b, a)),
+


Redundant empty line

SamVal007 added 2 commits February 17, 2021 20:34

Friends_APP

212e0dd

Friends_APP

83bf7c5

boxdxnx added the Friends App Task 15: Friends App label Feb 17, 2021

zonzujiro requested changes Feb 19, 2021

View reviewed changes

zonzujiro self-assigned this Feb 19, 2021

Update script.js

0f91fda

Made changes in order with review

d15ac84

- Add functions for sorting (more structural logic for sorting) - Changed long switch to more compact logic (with using addEventListener('change', ({target: radioButton}))

SamVal007 requested a review from zonzujiro February 27, 2021 22:54

zonzujiro requested changes Mar 2, 2021

View reviewed changes

SamVal007 added 3 commits March 28, 2021 05:59

Update script.js

7fe11b6

Made fixes by review.

Update index.html

9b4cd4d

Update script.js

354d014

SamVal007 requested a review from zonzujiro March 28, 2021 03:04

zonzujiro requested changes Apr 23, 2021

View reviewed changes


		const ageSorters = {
		ageLow: () => persons.sort((a, b) => compareAge(b, a)),

Uh oh!

Conversation

SamVal007 commented Feb 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamVal007 Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OleksiyRudenko commented Feb 26, 2021

Uh oh!

SamVal007 commented Feb 27, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zonzujiro commented Apr 11, 2021

Uh oh!

SamVal007 commented Apr 11, 2021 via email

Uh oh!

zonzujiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

SamVal007 Feb 27, 2021 •

edited

Loading