Skip to content

Week 3 & Week-3-practise#52

Open
sonmezali wants to merge 21 commits intorarmatei:masterfrom
sonmezali:week-3
Open

Week 3 & Week-3-practise#52
sonmezali wants to merge 21 commits intorarmatei:masterfrom
sonmezali:week-3

Conversation

@sonmezali
Copy link

No description provided.

Copy link
Collaborator

@tekul tekul left a comment

Choose a reason for hiding this comment

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

Good stuff Ali 👍 . No real problems with the coding 💯 . Just watch out for the names you choose for your functions. It can be difficult but try to choose a name which matches what the function does as much as possible.

return attendance[0];
} else {
return "";
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

When you write a function for use with filter or find, it should return true or false for each input, not a string. Don't try to rely on Javascript's odd behaviour. The filter and map are two separate things so shouldn't share the same function. It's better to have something like:

var eligibleStudentNames = attendances
  .filter(isEligibleStudent)
  .map(studentName)

];

var longNameThatStartsWithA = findLongNameThatStartsWithA(names);
function findLongNameThatStartsWithA(name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this function just checks a single value, it would be better named isLongNameThatStartsWithA. It doesn't do any finding itself. It could equally well be used with a filter call for example, so the context in which it is used shouldn't affect its name.

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