Skip to content

Phonebook #16

New issue

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

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

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Phonebook #16

wants to merge 2 commits into from

Conversation

NataliaButenko
Copy link
Collaborator

No description provided.

transformNumber(phone) {
if(this.checkForNumbers(phone)){
let arrPhone = phone.split('');
arrPhone.splice(0, 0, '(');
Copy link
Member

Choose a reason for hiding this comment

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

lets think can we apply regexp here, it would simplify that construction a lot

]
};

checkForNumbers(phone) {
Copy link
Member

Choose a reason for hiding this comment

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

probably in most cases there should be better name similar to

isPhoneNumberValid

meaning of "is" prefix at the beginning that function return boolean

let validNumber = numberArrPhone.some((elem) => {
return isNaN(elem)
});
if(validNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can just write that way

return validNumber

};

changeUserData(initialUser, value) {
let arr = this.dataBase;
Copy link
Member

Choose a reason for hiding this comment

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

it's hard to understand what does that function doing

phoneApp.addUser(3, 'Natalia', '0995305300');
phoneApp.removeUserByName('Tom2');
phoneApp.searchByName('Natalia');
phoneApp.changeUserData(3, 5);
Copy link
Member

Choose a reason for hiding this comment

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

not sure it easy to understand what 3 and 5 refers to.

I guess the public API(interface) should looks similar to that one

changeUser(userIdToChange, updatedUser) {
 
}

and inside of that method, you looking for the user with unique ID and than update it

let phoneApp = new PhoneApp();
console.log('проверенный и преобразованый номер --->', phoneApp.transformNumber('0995305385'));
console.log('номер не прошедший проверку --->', phoneApp.transformNumber('099530)5385'));
phoneApp.addUser(3, 'Natalia', '0995305300');
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's better to pass directly object with properties it would "self-documented-code"

};

addUser(id, name, phone) {
let objUser = {
Copy link
Member

Choose a reason for hiding this comment

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

You can create an additional class which would create such a user.

And if we go further maybe even phone-number validation should be made inside of User class.

Not sure if it the responsibility of phone-book to validate users.
The main responsibility of phone-book is to find users, update them and delete them

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