Skip to main content
added 337 characters in body
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

Thread-safety

The implementation is not thread-safe. checkstring calls functions that mutate the internal state of the instance (naturalDate and unixTime), so if this function gets called from concurrent threads, you may get incorrect results.

You can easily make it thread-safe by eliminating the naturalDate and unixTime fields. You don't need them. These can be local variables in checkstring, and passed as parameters to getResults.

In fact, the best would be to replace getResults with dedicated functions that return {unix: ?, natural: ?} with the appropriate value filled. (See further below.)

Design

Also, I would spell out nat as natural in unixToNat and natToUnix.

Thread-safety

It's true that thread-safety is not a practical concern in JavaScript, as user code is typically executed on a single thread in browsers and in node.js too. However, I think it's still a good idea to write thread-safe code when it's easily possible, in order to avoid picking up bad habits that may bite you later in other languages.

The implementation is not thread-safe. checkstring calls functions that mutate the internal state of the instance (naturalDate and unixTime), so if this function gets called from concurrent threads, you may get incorrect results.

You can easily make it thread-safe by eliminating the naturalDate and unixTime fields. You don't need them. These can be local variables in checkstring, and passed as parameters to getResults.

In fact, the best would be to replace getResults with dedicated functions that return {unix: ?, natural: ?} with the appropriate value filled, as I did in the examples above.

Thread-safety

The implementation is not thread-safe. checkstring calls functions that mutate the internal state of the instance (naturalDate and unixTime), so if this function gets called from concurrent threads, you may get incorrect results.

You can easily make it thread-safe by eliminating the naturalDate and unixTime fields. You don't need them. These can be local variables in checkstring, and passed as parameters to getResults.

In fact, the best would be to replace getResults with dedicated functions that return {unix: ?, natural: ?} with the appropriate value filled. (See further below.)

Design

Also, I would spell out nat as natural in unixToNat and natToUnix.

Design

Also, I would spell out nat as natural in unixToNat and natToUnix.

Thread-safety

It's true that thread-safety is not a practical concern in JavaScript, as user code is typically executed on a single thread in browsers and in node.js too. However, I think it's still a good idea to write thread-safe code when it's easily possible, in order to avoid picking up bad habits that may bite you later in other languages.

The implementation is not thread-safe. checkstring calls functions that mutate the internal state of the instance (naturalDate and unixTime), so if this function gets called from concurrent threads, you may get incorrect results.

You can easily make it thread-safe by eliminating the naturalDate and unixTime fields. You don't need them. These can be local variables in checkstring, and passed as parameters to getResults.

In fact, the best would be to replace getResults with dedicated functions that return {unix: ?, natural: ?} with the appropriate value filled, as I did in the examples above.

Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

Thread-safety

The implementation is not thread-safe. checkstring calls functions that mutate the internal state of the instance (naturalDate and unixTime), so if this function gets called from concurrent threads, you may get incorrect results.

You can easily make it thread-safe by eliminating the naturalDate and unixTime fields. You don't need them. These can be local variables in checkstring, and passed as parameters to getResults.

In fact, the best would be to replace getResults with dedicated functions that return {unix: ?, natural: ?} with the appropriate value filled. (See further below.)

Design

It's not great that some functions that manipulate internal state are exposed. It would be better to make them private, inaccessible from outside the class.

Alternatively, change them in a way that they will not modify the internal state.

Avoid unnecessary evaluation

Here, test2 doesn't need to be evaluated when test is true:

let format = /\w{3,9}?\s\d{1,2}?,\s\d{4}?/;
let unixFormat = /^\d[0-9]{0,20}$/;
let test = format.test(string);
let test2 = unixFormat.test(string);

if (test === true){
  this.natToUnix(string);

} else if (test2 === true) {
  this.unixToNat(string);

I would rewrite like this:

if (this.isNaturalFormat(input)) {
  return this.getResultWithUnixFormat(this.naturalToUnix(input));
}
if (this.isUnixFormat(input)) {
  return this.getResultWithNaturalFormat(this.unixToNatural(input));
}
return this.getResult();

All the functions I invented here, I suggest to write them ;-) That is, getResultWithUnixFormat takes as parameter a date in unix format, and returns {unix: ?}, and so on.

Use boolean values directly

Since .test(...) returns a boolean, you could use if (test) instead of this:

let test = format.test(string);
// ...

if (test === true){
  // ...

Naming

test and test2 don't tell much about their purpose:

let format = /\w{3,9}?\s\d{1,2}?,\s\d{4}?/;
let unixFormat = /^\d[0-9]{0,20}$/;
let test = format.test(string);
let test2 = unixFormat.test(string);

I suggest to rename them to isNaturalFormat and isUnixFormat.

Also, I would spell out nat as natural in unixToNat and natToUnix.