3
\$\begingroup\$

I have made a small timestamp microservice that converts from UNIX time to a natural language date, and vice versa. Please let me know of anything I could do better.

Here is a demo. Here is the time.js file that holds the class that controls converting the time:

let debug = require('debug')('test');
let moment = require('moment');


class TimeConverter {
  constructor(){
    this.naturalDate = null;
    this.unixTime = null;
  }
  //check if string meets a specific format
  checkString(string){
    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);

    } else {
      this.naturalDate = null;
      this.unixTime = null;
    }

    return this.getResults();

  }
  //convert natural language date to unix
  natToUnix(string){
    //put into iso format for moment.js
    let newDate = new Date(string);
    //change into unix time
    let date = moment(newDate).unix();
    this.unixTime = date;
    this.naturalDate = string;


  }
  //change unix time to natural language date
  unixToNat(string){

    let date = moment.unix(string);
    let formatted = date.format("MMMM DD YYYY");

    this.unixTime = string;
    this.naturalDate = formatted;


  }

  getResults(){
    let results = {unix: this.unixTime, natural: this.naturalDate};
    debug(results);
    return results;
  }



}

let time = new TimeConverter();

module.exports = time;

Here is the server.js file that has some express code. It handles the routes in the application. It grabs the input from the request parameters, and serves the CSS / index.html files:

  let express = require('express');
  let debug = require('debug')('test');

  let app = express();

  let bodyParser = require('body-parser');
  let time = require('./time.js');
  let port = process.env.PORT || 8080;

  app.use(bodyParser.json());
  //serves static files
  app.use('/static', express.static('public'));
  app.use(bodyParser.urlencoded({ extended: true}));
  //route for inputting the time
  app.get(['/:time','/'], function(req,res){
    res.sendFile(__dirname + "/index.html");
    let data = req.params;

    if (data.time != undefined){
        res.json(time.checkString(data.time));
    }

  });

  //tell server to listen on a port
  app.listen(port, function(){
    debug("listening on " + port);
  });

Here is the repo.

\$\endgroup\$
1
\$\begingroup\$

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.

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.

\$\endgroup\$
2
  • \$\begingroup\$ Since Javascript is single threaded, does thread-safety matter? I was trying to read about thread-safety with javascript, and other in other stackoverflow posts, people are saying to disregard it since javascript is single threaded. Is this true? I’m trying to understand thread safety more. Does thread safety have anything to do with preventing “side effects?” \$\endgroup\$ – Dream_Cap May 29 '17 at 0:27
  • \$\begingroup\$ @Dream_Cap you're right about thread-safety in JavaScript. I added more explanation to that section, and moved it from the top point to the bottom. \$\endgroup\$ – janos May 29 '17 at 5:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.