Skip to content
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

Use smart 'add' function #180

Merged
merged 1 commit into from
Feb 16, 2020
Merged

Use smart 'add' function #180

merged 1 commit into from
Feb 16, 2020

Conversation

Mudrekh
Copy link
Contributor

@Mudrekh Mudrekh commented Feb 15, 2020

@hawkeye64 Hello, I'm looking for an onvif project to build on and stumbled across your module and gave it a test spin. My initial tests were promising, so I figured I'd give a PR a shot.

This PR aims to simplify the module loading in Camera.js. The switch is a little overkill for something that is quite repetitive. Its nice to see that all your modules follow the same syntactic style!

@Mudrekh Mudrekh requested a review from hawkeye64 February 15, 2020 03:49
@Mudrekh
Copy link
Contributor Author

Mudrekh commented Feb 15, 2020

@hawkeye64 Looks like this PR failed at the coveralls step. Would love your input on that since I am not too familar with CircleCI/Coveralls

I ran `npm run jsdoc`...


Use after map object
@hawkeye64
Copy link
Owner

hawkeye64 commented Feb 15, 2020

I was such a noob when I wrote this. My first package. And, we still use it at work.
Now, when I look at it, I'd say what you did was definitely on the way of optimization and perf, but can be taken a step further.
Totally agree that the switch statement should disappear.
Instead, create an object with keys from the switch statement

let actions = {
  snapshot: {
    init () {
        if (!this.snapshot) {
          const Snapshot = require('./utils/snapshot')
          this.snapshot = new Snapshot()
          const defaultProfile = this.getDefaultProfile()
          if (defaultProfile) {
            const snapshotUri = defaultProfile.SnapshotUri.Uri
            this.snapshot.init(snapshotUri, this.username, this.password)
          }
        }
    }
  },
  media: {
    init () {
        if (!this.media) {
          const Media = require('./modules/media')
          this.media = new Media()
          this.media.init(this.timeDiff, this.serviceAddress, this.username, this.password)
        }
    }
  }
  // and so on
}

then,

add (name) {
  if (this.actions[name]) {
    this.actions[name].init()
  } else {
    // error handling
}

Do you want to try again? Maybe a different branch since this one is "polluted" now.

@Mudrekh
Copy link
Contributor Author

Mudrekh commented Feb 15, 2020

Sorry, what do you mean by a step further? And what do you mean by polluted?

@hawkeye64
Copy link
Owner

By "polluted" I mean, if you make more, radical changes, that are not in step with the current branch, then the PR looks messy.
By "step further" I mean do the code I suggested above. Or, wait until Monday when I have PTZ cameras available I can test with and I'll do it.

@hawkeye64
Copy link
Owner

Also, not sure why the Coveralls failed. It may only work for me. I am not sure.

/home/circleci/onvif-nvt/node_modules/coveralls/bin/coveralls.js:19
      throw err;
      ^
Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}

@Mudrekh
Copy link
Contributor Author

Mudrekh commented Feb 15, 2020

So you would want to still define a block/ funciton per module instead of using the string to require it? As for polluted... I can always just force push over it. Its just a fork.

@hawkeye64 hawkeye64 merged commit ec776f5 into hawkeye64:master Feb 16, 2020
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