HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Serial port for Node.js

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
nodeserialforport

Problem

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;

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#78845, answer score: 5

Revisions (0)

No revisions yet.