cleaned up address.js and util.js #96
Conversation
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
@ralphtheninja I agree. I think I moved it out of HD because I was used somewhere else at some point. Any suggestion for a better name?
There was a problem hiding this comment.
@ralphtheninja Could you remove the comments on this function? I don't want to put opinions into the code, I think this is better as a github issue or pull request. Thanks!
There was a problem hiding this comment.
Good point! I'll remove it.
There was a problem hiding this comment.
I don't have a better name for it. Need to read some more code and look at how it's used to have an opinion. I'll add a PR on it later.
|
Has it been decided to not use semicolons? I would expect the opposite. |
|
@symaxian It has been decided to not use semicolons, correct. |
|
I was under the assumption that they're highly highly recommended, especially for what needs to be "hardened" software. Is there a discussion of this posted on Github or somewhere else? |
|
They're not highly recommended, they are almost always unnecessary. http://mislav.uniqpath.com/2010/05/semicolons/ I've been given the green light by the other commiters to set a style for bitcoinjs-lib and I have decided that we will not use them. If there's an fringe semicolon issue it will almost certainly be caught by the tests (in node.js or the browsers with testling). You will find a lot of mission-critical software in node.js that do not have semicolons. Removing them is quickly becoming a standard in the JS community. |
|
|
||
| Address.getVersion = function(string) { | ||
| return base58.decode(string)[0]; | ||
| /** |
There was a problem hiding this comment.
What is the purpose of the empty comment area?
There was a problem hiding this comment.
Just for symmetry :) I can remove it if you wish.
|
Updated. Also rebased it against the latest master, so no multiple rail road tracks in the git history :)
Just noticed the tests are up to 130 now. Nice work! I'm gonna go through some code and see if there are any methods that needs testing. |
|
Note that this changes the api, since we no longer export the HmacFromBytesToBytes. I hope this doesn't cause any trouble. |
| : bytes.length <= 40 ? convert.hexToBytes(bytes) | ||
| : util.error('invalid or unrecognized input'); | ||
| /** | ||
| * Address constructor. Call it with new, or don't. |
There was a problem hiding this comment.
Could you remove this comment? After that I think we're golden. Thanks!
|
Actually. How about changing the 'bytes' parameter to 'address'? Because that's what it is. |
|
The address constructor is overloaded -- it accepts bytes, Address and string. I'd like to move away from overloaded constructors and instead towards empty(or bytes arguments) for constructors, and turn the other forms into separate class methods. An example where this is implemented is hdwallet.js https://github.com/bitcoinjs/bitcoinjs-lib/blob/master/src/hdwallet.js#L39-L45 |
cleaned up address.js and util.js
|
@weilu completely agree with removing the overloaded constructors. |
|
+1 for removing overloaded constructors. It makes the code much harder to understand if a parameter can be many different things. |
|
I agree as to the overloaded constructors, the explicit instance methods would result in more readable code. |
Two space indentation, got rid of semi colons and some refactoring. Tests pass. Are you people ok with this format?