Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fiqare secmotic rules #411

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
4 changes: 3 additions & 1 deletion lib/bindings/HTTPBindings.js
Original file line number Diff line number Diff line change
@@ -57,6 +57,7 @@ function handleError(error, req, res, next) {
name: error.name,
Copy link
Member

@fgalan fgalan Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR is almost ready (good work! :). However, you should include an entry in the CHANGES_NEXT_RELEASE file in describing the changes. Maybe something like the following:

- Hardening: software quality improvement based on ISO25010 recommendations

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 484e833

message: error.message
});
next();
}

function parseData(req, res, next) {
@@ -159,7 +160,7 @@ function returnCommands(req, res, next) {
updates = commandList.map(createCommandUpdate);
cleanCommands = commandList.map(cleanCommand);

async.parallel(updates.concat(cleanCommands), function(error, results) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

results seems a reminder of a log that was necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case the fix should be to use results to provide a proper log messages based on it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we continue to rely on ISO 25010 to improve software quality. Mainly:

Rule 1: Unused function parameters should be removed, based on:

MISRA C++:2008, 0-1-11 - There shall be no unused parameters (named or unnamed) in nonvirtual functions.
MISRA C:2012, 2.7 - There should be no unused parameters in functions
CERT, MSC12-C. - Detect and remove code that has no effect or is never executed

So, this is fixed in a2e11f6

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agree that unused parameters should be avoided. However, we are proposing a different solution for this case: instead of removing the unused results parameter, let's add a log trace that uses results to print some useful information in the logs for the user.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the maintainability axis of ISO 25010, I think it is counterproductive to add code to the function to use a variable that is currently deprecated, which makes it less readable. Therefore, before adding code that will not be functional, I prefer to leave the function as it was, with the variable "results".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 9dfbdcb

async.parallel(updates.concat(cleanCommands), function(error) {
if (error) {
// prettier-ignore
config.getLogger().error(
@@ -203,6 +204,7 @@ function returnCommands(req, res, next) {
} else {
res.status(200).send('');
}
next();
}

function handleIncomingMeasure(req, res, next) {
2 changes: 1 addition & 1 deletion lib/bindings/MQTTBinding.js
Original file line number Diff line number Diff line change
@@ -278,7 +278,7 @@ function start(callback) {
}
});
mqttClient.on('message', commonBindings.mqttMessageHandler);
mqttClient.on('connect', function(ack) {
mqttClient.on('connect', function() {
config.getLogger().info(context, 'MQTT Client connected');
recreateSubscriptions();
});
3 changes: 1 addition & 2 deletions lib/commonBindings.js
Original file line number Diff line number Diff line change
@@ -112,10 +112,9 @@ function manageConfigurationRequest(apiKey, deviceId, device, objMessage) {
* @param {String} apikey APIKey of the service the device belongs to.
* @param {Array} previous Array of prepared functions that send information to the Context Broker.
* @param {Object} current Information sent by the device.
* @param {Number} index Index of the group in the array.
* @return {Array} Updated array of functions.
*/
function processMeasureGroup(device, apikey, previous, current, index) {
function processMeasureGroup(device, apikey, previous, current) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaDoc entry for index (L115) should be removed also.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 662a458

var values = [];

if (current.command) {
4 changes: 4 additions & 0 deletions lib/iotaUtils.js
Original file line number Diff line number Diff line change
@@ -154,6 +154,10 @@ function mergeDeviceWithConfiguration(deviceData, configuration, callback) {
deviceData[fields[i]] = configuration[confField];
} else if (!deviceData[fields[i]] && (!configuration || !configuration[confField])) {
deviceData[fields[i]] = defaults[i];
} else {
config
.getLogger()
.debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything', i);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything', i);
.debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything');

not fully sure but I'd say that if you don't use formating (such as "%d") then the i variable should not be included at the end of the statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 9dfbdcb

}

if (deviceData[fields[i]] && ['active', 'lazy', 'commands'].indexOf(fields[i]) >= 0) {