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

Code correctness - setters don't return a value #280

Closed
Rycochet opened this issue Mar 10, 2017 · 2 comments
Closed

Code correctness - setters don't return a value #280

Rycochet opened this issue Mar 10, 2017 · 2 comments

Comments

@Rycochet
Copy link
Contributor

I'm creating a TypeScript definition file (will push back into the project when done), and noticed that every setter is also returning the value set - this does nothing and is unused, so the return itself can be removed (it's just wasting space and potentially creating confusion):

IE. https://github.com/guyonroche/exceljs/blob/master/lib/doc/cell.js#L85

  get numFmt() {
    return this.style.numFmt;
  },
  set numFmt(value) {
    return this.style.numFmt = value;
  },

...should be...

  get numFmt() {
    return this.style.numFmt;
  },
  set numFmt(value) {
    this.style.numFmt = value;
  },

A cascading sum like a = b = c will call the getter on c, then the setter on b, then the getter on b, and finally the setter on a.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/set

(Although it's not directly clear from the docs, there is an explicit return in the getter, and none in the setter, in addition to all the examples.)

@guyonroche
Copy link
Collaborator

@Rycochet thanks for the tip - returning values from setters is a habit from my C++ days, I hadn't realised they were being ignored.
I've removed returns from setters and pushed to the repo

@Rycochet
Copy link
Contributor Author

@guyonroche I know the feeling, hence checking the references, several hundred bytes of savings are always good ;-)

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

No branches or pull requests

2 participants