Skip to content

Conversation

SergeyKostikov
Copy link
Collaborator

No description provided.

.split("")
.splice(0, 3)
.join("");
let resultPhone =
Copy link
Member

Choose a reason for hiding this comment

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

what about regExp ?

};

deleteUser(id) {
this.database.forEach((elem, index) => {
Copy link
Member

Choose a reason for hiding this comment

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

such case would be much better

this.database = this.database.filter( ... )

this.database.splice(index, 1);
}
});
this.database.splice();
Copy link
Member

Choose a reason for hiding this comment

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

a lot of splices here

this.database.splice();
};
searchUserByName(name) {
return this.database.reduce(function(newElem, elem) {
Copy link
Member

Choose a reason for hiding this comment

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

use Array.prototype.filter is fine here and much better than reduce

editUser(id, options) {
this.database.map(elem => {
if (elem.id == id) {
if (options.name) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible use don't have such property?

sortUser(param, direction) {
return this.database.sort((a, b) => {
if (direction) {
if (direction == "big") {
Copy link
Member

Choose a reason for hiding this comment

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

what does small/big refer to ? :)

if (options.name) {
user.name = options.name;
}
if (options.phone) {
Copy link
Member

Choose a reason for hiding this comment

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

please omit code duplication, there the same code at line 14

}
this.database.push(user);
};
checkPhoneNumber(phone){isNaN(+phone)};
Copy link
Member

Choose a reason for hiding this comment

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

please make multi-line

this.database = [];
}

addUser (options) {
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 method addUser accepts 'user' as param

})

const myPhoneApp = new PhoneApp();
myPhoneApp.addUser(vasya);
Copy link
Member

Choose a reason for hiding this comment

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

a pretty interesting decision to create user outside of phoneApp.

But it will be even better to make additional "mental-layer" to create such users. That layer would validate and than send user to phoneApp the main goal of such approach to make sure DATA(users) already validated and we just want to operate with them, without any validation inside internal methods of PhoneApp

<div class="app-header">
My Phone App
</div>
<div class='app-content'>
Copy link
Member

Choose a reason for hiding this comment

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

you have to use double quotes for attributes, it could cause problems in some browser

</div>
<div class='app-content'>
<div class='app-contact-list'>
<table class='contact-table' cellspacing="0" cellpadding="0">
Copy link
Member

Choose a reason for hiding this comment

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

same here about quotes

// myPhoneApp.editUser(2, { name: "Voljin" });
// myPhoneApp.editUser(4, { homePhone: "159357" });
// console.log(myPhoneApp.filterUser("homePhone"));
// console.log(myPhoneApp.sortUser("phone", "big"));
// console.log(myPhoneApp.checkAndFormatPhoneNumber("0993378130"));
Copy link
Member

Choose a reason for hiding this comment

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

let clean up it a bit, please remove unused parts of the code

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