search

googleinterns / smart-home-local

Apache License 2.0
3 stars 2 forks source link

Post-merge review, part 1 of N #42

Open limlam opened 4 years ago

limlam commented 4 years ago

Hey Colin,

Sorry for the delay in reviewing your changes!

I have some questions about what you have so far and I'm going to put them all here for convenience. Feel free to open new issues based off some commentary here.

device-manager.ts

Nits

1) Order class constants alphabetically 1) Add newline after block of class constants

Style

I'm not familiar with the Google JavaScript Style Guide so forgive these questions: 1) Is it typical to declar class private members first? 1) This next one might be more a syntax question vs a style question, but what is the code block starting here doing?

Code

1) I don't see any usages of public accessors getCommandsSent and wasCommandSent. I typically prefer to follow two principals here: 1) Production code is used in production and tests, not only in tests 1) YAGNI (You Aren't Gonna Need It)

1) If you are going to keep getCommandsSent as a public accessor, please change it to match the member name with a get as a prefix. 1) And while we're thinking about it, I'm not exactly following why it's necessary to keep a set of commands around. In Local Home Platform, we can deal with Commands in parallel, but we prefer not to encourage JavaScript behavior that disrupts an asynchronous Promise chain.

1) The addExpectedCommand sounds like we're walking down the path of creating a mock infrastructure. Rather, we want to create a fake/stub. The caller of addExpectedCommand shouldn't know what to pass in as the command response. A combo of MockLocalHomePlatform and DeviceManagerStub should figure out what to do.

cd9 commented 4 years ago

Hey Manit, thanks for the comprehensive review. I responded to your points and opened a few issues:

Nits

  • Order class constants alphabetically

Ack. Opened #43

  • Add newline after block of class constants

Ack. Opened #44

Style

  1. Is it typical to declar class private members first?

According to the Google JavaScript Style Guide, "There is no hard and fast rule governing how to order the members of a class". I typically try to declare private members first, but I have no strong argument for it so let me know if you have a general rule I should follow and I'll open an issue.

2.This next one might be more a syntax question vs a style question, but what is the code block starting here doing?

The below two union type declarations are equivalent.

type A = B | C;
// OR
type A =
  | B
  | C

This was was auto-formatted by the prettier rules for Google TypeScript Style

  1. The line wrapping for the definition of Map and Set look odd

I agree these look strange, but this was auto-formatted by the Google TypeScript Style fixer rules, so I'm inclined to leave it. If you think it's worth overriding these rules, I can make the change.

Code

I don't see any usages of public accessors getCommandsSent and wasCommandSent. I typically prefer to follow two principals here: Production code is used in production and tests, not only in tests YAGNI (You Aren't Gonna Need It)

Ack. Will keep it in mind as general principles from now on.

If you are going to keep getCommandsSent as a public accessor, please change it to match the member name with a get as a prefix.

Ack and opened issue #45. Will go through and enforce this.

And while we're thinking about it, I'm not exactly following why it's necessary to keep a set of commands around. In Local Home Platform, we can deal with Commands in parallel, but we prefer not to encourage JavaScript behavior that disrupts an asynchronous Promise chain.

The buffer of sent commands was added to allow the developer to test if their executeHandler had or hadn't sent a command to DeviceManager. If their test fails, they could potentially compare the Command that was sent against the Command they expected their executeHandler to send. I will explore alternative implementations for this.

The addExpectedCommand sounds like we're walking down the path of creating a mock infrastructure. Rather, we want to create a fake/stub. The caller of addExpectedCommand shouldn't know what to pass in as the command response. A combo of MockLocalHomePlatform and DeviceManagerStub should figure out what to do.

Some questions/thoughts on this: