5
\$\begingroup\$

I am trying to create a class for serial port using Node.js, but I want to know if there is a better way to write my data class code.

In my sample code below, in the getPortName prototype in the forEach loop, I declare a port object to store the serial port information before pushing into an array.

Is this a good way of declaring this port object? I think it is not good because it is difficult for users to see what the objects' members contain. Is there any way to make it clearly expose the port object member? Or is there a better way to improve this code structure?

'use strict';

var serialPort = require("serialport");

function Serial() {
    this.ComName = "";
}

Serial.prototype.getPortName = function() {
    serialPort.list(function (err, ports) {
        var availablePorts = [];    

        ports.forEach(function(port) {
            var port = {
                 comName: "",
                 manufacturer: "",
                 pnpId: 
            }

            port.comName = port.comName;
            port.manufacturer = port.manufacturer;
            port.pnpId = port.pnpId;
            availablePorts.push(port);
        });
    });

    return availablePorts;
};

Serial.prototype.setCOMName = function(name) {
    this.ComName = name;
}

module.exports = Serial;
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

First off, you should probably allow a default ComName to be passed into the constructor, and the default should probably be null instead of an empty string (unless you have good reason to do otherwise).

function Serial(name) {
    this.ComName = name !== undefined ? name : null;
}

Next, getPortName is strange. You set it on Serial.prototype, which indicates it will be used as an instance method. In fact, however, it doesn't use instance state at all! As your code is currently laid-out, you would need to perform the following to call getPortName:

new Serial().getPortName()

In classical OOP parlance, this is a static function. It doesn't need instance state, so declare it on Serial itself, not Serial.prototype.

Serial.getPortName = function() { ... }

With that change, you can simply call Serial.getPortName(), as you would a static method in other languages.

Furthermore, your naming conventions are a little odd. You use ComName for the property but COMName for the setter. Use comName for the property for consistency. If you need the setter, use setComName. However, the setter is also unnecessary. Why not just access serial.comName directly?

Finally, these changes beg the question... why is this object-oriented code at all? The only state you appear to be representing is a single string with no actual methods on it. Why not just export getPortName and call it a day? If your code is more complex, using JavaScript's OO capabilities might be worth it, but otherwise, you don't need it. A simple function will do.

\$\endgroup\$
2
  • \$\begingroup\$ thank you for your clear explanation, i have question about your setter and getter, you mention that i could just access it directly instead of using setter or getter, but i do read that it is beneficial for me to use setter and getter for my class member? 2nd) how do i represent my data class (the port object contains name, pnpid..) how could i make this data class clear and be reused? \$\endgroup\$
    – Tim
    Commented Jan 28, 2015 at 6:27
  • \$\begingroup\$ @Tim That's a number of questions, and I don't plan to answer them in the comments of this answer. I will say that with the near-ubiquitous support for Object.defineProperty, using custom properties is probably a better replacement for getters and setters most of the time. \$\endgroup\$ Commented Jan 28, 2015 at 6:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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