ES6: migrated "entity" (part 2) #1195

bennycode · 2017-05-08T23:07:36Z

Type of change

Screenshot

gregor · 2017-05-09T08:26:23Z

app/script/entity/Conversation.js

+window.z = window.z || {};
+window.z.entity = z.entity || {};
+
+


One too many

gregor · 2017-05-09T08:26:55Z

app/script/entity/Conversation.js

+    this.name = ko.observable();
+    this.input = ko.observable(z.util.StorageUtil.get_value(`${z.storage.StorageKey.CONVERSATION.INPUT}|${this.id}`) || '');
+    this.input.subscribe((text) => {
+      return z.util.StorageUtil.set_value(`${z.storage.StorageKey.CONVERSATION.INPUT}|${this.id}`, text);


I guess we can keep simply things like this in a single line

It extended our character limit per line:

gregor · 2017-05-09T08:27:07Z

app/script/entity/Conversation.js

+    this.participating_user_ids = ko.observableArray([]);
+    this.self = undefined;
+    this.number_of_participants = ko.pureComputed(() => {
+      return this.participating_user_ids().length;


gregor · 2017-05-09T08:28:04Z

app/script/entity/Conversation.js

+          return this.participating_user_ets()[0].name();
+        }
+        return '…';
+      } else if (this.is_group()) {


No need for else. You always return in the above if

gregor · 2017-05-09T08:29:06Z

app/script/entity/Conversation.js

+  add_message(message_et) {
+    amplify.publish(z.event.WebApp.CONVERSATION.MESSAGE.ADDED, message_et);
+    this._update_last_read_from_message(message_et);
+    return this.messages_unordered.push(this._check_for_duplicate_nonce(message_et, this.get_last_message()));


Don't return if you return undefined

gregor · 2017-05-09T08:29:22Z

app/script/entity/Conversation.js

+      }
+    }
+
+    return z.util.ko_array_push_all(this.messages_unordered, message_ets);


Do we really want to return this?

gregor · 2017-05-09T08:29:32Z

app/script/entity/Conversation.js

+      message_et = this._check_for_duplicate_nonce(message_ets[index - 1], message_et);
+    }
+
+    return z.util.ko_array_unshift_all(this.messages_unordered, message_ets);


gregor · 2017-05-09T08:29:52Z

app/script/entity/Conversation.js

+   * @returns {undefined} No return value
+   */
+  remove_message(message_et) {
+    return this.messages_unordered.remove(message_et);


Sure we want to return this?

gregor · 2017-05-09T08:29:59Z

app/script/entity/Conversation.js

+   * @returns {undefined} No return value
+   */
+  remove_messages() {
+    return this.messages_unordered.removeAll();


gregor · 2017-05-09T08:30:20Z

app/script/entity/Conversation.js

+   * @note If a message is send to the backend multiple times by a client they will be in the conversation multiple times
+   * @param {z.entity.Message} message_et - Message entity to be added to the conversation
+   * @param {z.entity.Message} other_message_et - Other message entity to compare with
+   * @returns {undefined} No return value


You return a message entity

Not anymore 🗡

gregor · 2017-05-09T08:30:34Z

app/script/entity/Conversation.js

+   * Creates the placeholder message after clearing a conversation.
+   * @private
+   * @note Only create the message if the group participants have been set
+   * @returns {undefined} No return value


You return a message entity

Not anymore 🗡

gregor · 2017-05-09T08:30:45Z

app/script/entity/Conversation.js

+   */
+  update_latest_from_message(message_et) {
+    if ((message_et != null) && message_et.visible() && message_et.should_effect_conversation_timestamp) {
+      return this.set_timestamp(message_et.timestamp(), z.conversation.ConversationUpdateType.LAST_EVENT_TIMESTAMP);


gregor · 2017-05-09T08:30:51Z

app/script/entity/Conversation.js

+    const has_timestamp = message_et.timestamp();
+
+    if (is_me && has_timestamp && message_et.should_effect_conversation_timestamp) {
+      return this.set_timestamp(message_et.timestamp(), z.conversation.ConversationUpdateType.LAST_READ_TIMESTAMP);


gregor · 2017-05-09T08:31:36Z

app/script/entity/Conversation.js

+    return [this.self]
+      .concat(this.participating_user_ets())
+      .filter((user_et) => !user_et.is_verified())
+      .map((user_et) => user_et);


No need for this map

gregor

Please check JSDoc returns and your actual returns

ffflorian · 2017-05-09T08:31:15Z

app/script/entity/Conversation.js

+    // Conversation states for view
+    this.is_muted = ko.pureComputed(() => {
+      return this.muted_state();
+    });


I'd suggest

this.is_muted = ko.pureComputed(() => this.muted_state());

everywhere where functions consist only of one return line.

bennycode · 2017-05-09T08:49:32Z

@gregor & @ffflorian: I fixed all of your requests.

ffflorian

There are more one-liner functions missing which can/should be shortened.

Just replace \) => \{\n +return (.*);\n +\}\); with \) => $1\); 🙂

ffflorian · 2017-05-09T08:53:42Z

app/script/entity/Conversation.js

+      this.verification_state,
+    ].forEach((property) => {
+      return property.subscribe(this.persist_state);
+    });


].forEach((property) => property.subscribe(this.persist_state));

ffflorian · 2017-05-09T08:54:15Z

app/script/entity/User.js

+    this.name = ko.observable('');
+    this.first_name = ko.pureComputed(() => {
+      return this.name().split(' ')[0];
+    });


this.first_name = ko.pureComputed(() => this.name().split(' ')[0]);

bennycode added 7 commits May 8, 2017 15:26

Connection

06869a6

User

0b8a4b5

User

30c41ce

User

687db72

Started with "Conversation"

aacb8c7

Transpiling done

8146b40

impro

451c694

lipis mentioned this pull request May 8, 2017

Drop CoffeeScript #891

Closed

79 tasks

JSDoc

22d57f8

gregor reviewed May 9, 2017

View reviewed changes

app/script/entity/Conversation.js Outdated

window.z = window.z || {};

window.z.entity = z.entity || {};

Copy link

Contributor

gregor May 9, 2017

Choose a reason for hiding this comment

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

One too many

gregor reviewed May 9, 2017

View reviewed changes

gregor suggested changes May 9, 2017

View reviewed changes

ffflorian suggested changes May 9, 2017

View reviewed changes

bennycode added 3 commits May 9, 2017 10:45

PR review

569a10e

style

b925ce0

style

d9e6284

ffflorian suggested changes May 9, 2017

View reviewed changes

bennycode added 3 commits May 9, 2017 11:14

prr

ff8e6f6

one-liners

1f3e640

Merge branch 'dev' into es6/entity

3e1e089

ffflorian approved these changes May 9, 2017

View reviewed changes

gregor approved these changes May 9, 2017

View reviewed changes

bennycode merged commit d2eea74 into dev May 9, 2017

bennycode deleted the es6/entity branch May 9, 2017 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES6: migrated "entity" (part 2) #1195

ES6: migrated "entity" (part 2) #1195

bennycode commented May 8, 2017

gregor May 9, 2017

gregor May 9, 2017

bennycode May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

bennycode May 9, 2017

gregor May 9, 2017

bennycode May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor May 9, 2017

gregor left a comment

ffflorian May 9, 2017

bennycode commented May 9, 2017

ffflorian left a comment •

edited

Loading

ffflorian May 9, 2017

ffflorian May 9, 2017

ES6: migrated "entity" (part 2) #1195

ES6: migrated "entity" (part 2) #1195

Conversation

bennycode commented May 8, 2017

Type of change

Screenshot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennycode commented May 9, 2017

ffflorian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffflorian left a comment •

edited

Loading