Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add tiny-js-world homework #193

Merged
merged 3 commits into from
Aug 18, 2022
Merged

add tiny-js-world homework #193

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7972a8d
add tiny-js-world homework
bmukha Aug 16, 2022
7c0cc13
refactored according to mentor's suggestions
bmukha Aug 17, 2022
f251e83
refactored forEach
bmukha Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions submissions/bmukha/tiny-js-world/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/* Refer to https://github.com/OleksiyRudenko/a-tiny-JS-world for the task details
Complete the below for code reviewers' convenience:

Code repository: https://github.com/bmukha/a-tiny-JS-world
Web app: https://bmukha.github.io/a-tiny-JS-world/
*/

const dog = {
species: 'dog',
name: 'Beethoven',
gender: 'male',
legs: 4,
hands: 0,
saying: 'woof',
friends: ['Clyde'],
};

const cat = {
species: 'cat',
name: 'Grizabella',
gender: 'female',
legs: 4,
hands: 0,
saying: 'meow',
friends: ['Bonnie'],
};

const man = {
species: 'human',
name: 'Clyde',
gender: 'male',
legs: 2,
hands: 2,
saying: 'Get rich or die trying!',
friends: ['Bonnie', 'Beethoven'],
};

const woman = {
species: 'human',
name: 'Bonnie',
gender: 'female',
legs: 2,
hands: 2,
saying: 'I have the right to not answer a questions!',
friends: ['Clyde', 'Grizabella'],
};

const catwoman = {
species: 'human',
name: 'Selina',
gender: 'female',
legs: 2,
hands: 2,
saying: cat.saying,
friends: [],
};

const characters = [dog, cat, man, woman, catwoman];

const props = [
'species',
'name',
'gender',
'legs',
'hands',
'saying',
'friends',
];

characters.forEach((character) => {
const noZeroHandsOrFriends = props.filter(
(prop) =>
!(
(prop === 'hands' && character[prop] === 0) ||
(Array.isArray(character[prop]) && character[prop].length === 0)
)
);
const values = noZeroHandsOrFriends.map((prop) =>
Array.isArray(character[prop])
? character[prop].join(', ')
: character[prop]
);
Copy link
Member

@OleksiyRudenko OleksiyRudenko Aug 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do not change anything yet. At this stage please just answer questions and reflect on statements.

  1. Describe in plain language (any but just plain) what would be the rules to print the value of any particular property?

  2. Do we want to skip printing 0 only for hands? What if a character has 0 legs and 0 hands but 4 paws? What if I as a peer developer assigned to this project pass undefined instead of 0?

  3. Array methods that return arrays are chainable.
    map and filter always do. reduce may return an array, thus chainable by other Array methods.
    This means that we can do filter(...).map(...).filter(...).map(...) or filter(...).filter(...).map(...) or map(...).filter(...).filter(...).map(...) etc.

  4. The tinier the job a callback does the more readable and scalable the code is.
    e.g.

arr
  .filter(v => v !== 0)
  .filter(v => v !== "")
  .filter(v => Array.isArray(v) && v.length)

is cleaner and easier to extend and modify than
arr.filter(v => !(v=== 0 || v === "" || (Array.isArray(v) && v.length))

  1. Probably, we want to discard not only zero values, but also undefined, null and "" (empty strings).
    Check what Boolean(v) does when v is either of 0, "", undefined, null, 111, "abc", [], [1,2,3].
    Check what is the result of [].join("whatever glue string").

  2. Check what are the results of the statements below.

let v = 5;

v.length;
v?.length;

Explain.

  1. How could you leverage the learnings to simplify data checks?

P.S. You may quote-reply and comment inline or refer to item numbers in your response. Please feel free to answer those one by one in separate comments and in any order.

P.P.S. We won't necessarily implement every learning.

P.P.P.S. Apologies for many updates. Just a really good case to dig deeper into the topic of data processing and validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First of all, there is no need to apologise. I am here to learn and I appreciate your feedback a lot. Thank you very much for your time and effort.

0 and 1. I derived my decisions on what to filter out from those two rules:

if inhabitant has no hands then skip it or report 0 or undefined, it is totally up to you
Optional: each inhabitant can be friendly to 1 or more other inhabitants (or to none). If you implement this then the output should also list friends, i.e. human; John; male; 2; 2; Hello world!; Rex, Tom, Jenny

In the first rule, only hands were mentioned as questionable property. As for why I decided to filter them out only if hands === 0, well, I guess I really didn't think it through. From all options "skip it or report 0 or undefined" I've decided to go for the skip. And, in the second rule, if the friends array is empty, I decided to filter it out too for consistency. So, my interpretation was that only hands need to be checked, so that's why I did not go for some generalised solution.

  1. I am aware of chaining methods, but it is still more readable for me as a novice to write code without it. But I got the point, should probably try to get used to it.

  2. This one is really interesting to me. I'm not trying to argue, I'm trying to understand. Your way code loops over the array 3 times, while my way it loops only once. Genuine question: is it worth sacrificing a little bit of performance to achieve better readability? Should I always go with the readability approach, or does it depend on the situation?

  3. Again, I am aware of what Boolean(v) does. But I can't think of a clean and elegant solution from the top of my head to filter everything I need to filter at once. While I can filter out all falsy values with it, the problem is empty array is still a truthy value.

  4. I’ve done some research, and it looks like optional chaining. Totally new concept to me, since when I first tried to learn JS for my personal needs many years ago it wasn't even been introduced yet. I had to even update my Node from version 12 to 16 to make it work in my console, lol ) So, both examples should return undefined. But, if I understood everything correctly, the second one is useful when we need to check if some property exists and only then check its value.

  5. So, to summarize it all: for each character, we need to filter out all the properties with falsy values and properties which are empty arrays. (I still can’t see if it could be done in one clean and concise check, though). Then we need to map through the remaining properties to join them together (and join non-empty array properties in the process). So, I guess, the code should look something like this:

characters.forEach((character) => {
  print(
    props
      .filter((prop) => Boolean(character[prop]))
      .filter((prop) => character[prop]?.length !== 0)
      .map((prop) =>
        Array.isArray(character[prop])
          ? character[prop].join(', ')
          : character[prop]
      )
      .join('; ')
  );
});

Copy link
Member

Choose a reason for hiding this comment

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

You are right, splitting one complex filter condition into consequent filter's with a simpler logic each affects performance. So, readability takes it cost (2 filters are slower by 30-50% compared with a single filter).

Consider logic as complex as (a || !(b[i] || c) && d[i][j]) || (e ? f[i+1]:g). Same refers to nested if/else's. Quite easy to make a mistake when need to change anything and a nightmare to debug.

If we can improve readability by other means we should do.

Your latest example contains two simple conditions, so we can already combine them both in a single filter condition with not much impact on performance and yet quite clean and readable.

One important observation is that now your code works with values only, as we do not need property checks anymore. Makes sense to convert property names into values on some very early stage and you need arrays converted to strings anyway. Check [].join('any delimiter'). The benefit: you reduce a number of data types to analyze and work with further down. A good chance to simplify value checks even further.

We also get rid of character[prop] on early stage and may use simple value further down the chain. Not much to save in this case but think of it as of a pattern for more complex cases.

In few words, simplify, minimize, reduce complex data structures and logic as early as possible.

If you feel confident as to the next code improvement iteration then go ahead. Feel free to ask further questions and sharing your ideas.

P.S. Do not use x?.prop here, stick to Array.isArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One important observation is that now your code works with values only, as we do not need property checks anymore. Makes sense to convert property names into values on some very early stage and you need arrays converted to strings anyway.

Well, that was really helpful. By mapping first I can convert possible truthy empty arrays into falsy empty strings and then get rid of all the falsy values with one filter. Thank you very much for this suggestion! I just submitted refactored version, please, take a look.


print(values.join('; '));
});