From d3970f2cce4380137048d89b01b25d2aa1abc507 Mon Sep 17 00:00:00 2001
From: athatheo <vostidi@hotmail.com>
Date: Sun, 7 Apr 2019 17:50:38 +0300
Subject: [PATCH] Fixed stuff from the review - changed variable names - added
breaks - corrected a comment - deleted explicit declaration of true/false in
ifs - fixed a bug in tests caused by passing the message by reference
---
inc/Services/EventActionService.hpp | 2 +-
src/Services/EventActionService.cpp | 58 +++++++++++++---------------
test/Services/EventActionService.cpp | 8 +++-
3 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/inc/Services/EventActionService.hpp b/inc/Services/EventActionService.hpp
index b7e9b6d3..f774a1c4 100644
--- a/inc/Services/EventActionService.hpp
+++ b/inc/Services/EventActionService.hpp
@@ -12,7 +12,7 @@
*
* ECSS 8.19 && 6.19
*
- * Note: Make sure the check the note in the addEventActionDefinition()
+ * Note: Make sure to check the note in the addEventActionDefinition()
* Note: A third variable was added, the eventActionDefinitionID. This was added for the purpose of identifying
* various eventActionDefinitions that correspond to the same eventDefinitionID. The goal is to have multiple actions
* be executed when one event takes place. This defies the standard.
diff --git a/src/Services/EventActionService.cpp b/src/Services/EventActionService.cpp
index bdcad7f0..9aee53f1 100644
--- a/src/Services/EventActionService.cpp
+++ b/src/Services/EventActionService.cpp
@@ -6,25 +6,24 @@
void EventActionService::addEventActionDefinitions(Message& message) {
// TC[19,1]
message.assertTC(19, 1);
- message.resetRead();
uint16_t applicationID = message.readEnum16();
uint16_t eventDefinitionID = message.readEnum16();
uint16_t eventActionDefinitionID = message.readEnum16();
- bool flag = true; // This variable checks if the message can be added or not. Couldn't think of a better name
+ bool canBeAdded = true;
if (eventActionDefinitionMap.find(eventDefinitionID) != eventActionDefinitionMap.end()) {
auto range = eventActionDefinitionMap.equal_range(eventDefinitionID);
for (auto& element = range.first; element != range.second; ++element) {
if (element->second.eventActionDefinitionID == eventActionDefinitionID) {
- flag = false;
+ canBeAdded = false;
ErrorHandler::reportError(message, ErrorHandler::EventActionEventActionDefinitionIDExistsError);
}
}
}
if (message.dataSize - 6 > ECSS_TC_REQUEST_STRING_SIZE) {
- flag = false;
+ canBeAdded = false;
ErrorHandler::reportInternalError(ErrorHandler::MessageTooLarge);
}
- if (flag){
+ if (canBeAdded) {
char data[ECSS_TC_REQUEST_STRING_SIZE];
message.readString(data, message.dataSize - 6);
EventActionDefinition temp;
@@ -39,9 +38,8 @@ void EventActionService::addEventActionDefinitions(Message& message) {
void EventActionService::deleteEventActionDefinitions(Message& message) {
message.assertTC(19, 2);
- message.resetRead();
uint16_t numberOfEventActionDefinitions = message.readUint16();
- bool eventActionDefinitionIDexists = false;
+ bool definitionIDexists = false;
for (uint16_t i = 0; i < numberOfEventActionDefinitions; i++) {
message.skipBytes(2);
uint16_t eventDefinitionID = message.readEnum16();
@@ -49,16 +47,16 @@ void EventActionService::deleteEventActionDefinitions(Message& message) {
if (eventActionDefinitionMap.find(eventDefinitionID) != eventActionDefinitionMap.end()) {
auto range = eventActionDefinitionMap.equal_range(eventDefinitionID);
for (auto& element = range.first; element != range.second; ++element) {
- if (element->second.eventActionDefinitionID == eventActionDefinitionID){
- eventActionDefinitionIDexists = true;
- if (element->second.enabled == true){
+ if (element->second.eventActionDefinitionID == eventActionDefinitionID) {
+ definitionIDexists = true;
+ if (element->second.enabled) {
ErrorHandler::reportError(message, ErrorHandler::EventActionDeleteEnabledDefinitionError);
} else {
eventActionDefinitionMap.erase(element);
}
}
}
- if (eventActionDefinitionIDexists == false){
+ if (not definitionIDexists) {
ErrorHandler::reportError(message, ErrorHandler::EventActionUnknownEventActionDefinitionIDError);
}
} else {
@@ -78,23 +76,23 @@ void EventActionService::deleteAllEventActionDefinitions(Message& message) {
void EventActionService::enableEventActionDefinitions(Message& message) {
// TC[19,4]
message.assertTC(19, 4);
- message.resetRead();
uint16_t numberOfEventActionDefinitions = message.readUint16();
if (numberOfEventActionDefinitions != 0) {
for (uint16_t i = 0; i < numberOfEventActionDefinitions; i++) {
- message.skipBytes(2);
+ message.skipBytes(2); // Skips reading the application ID
uint16_t eventDefinitionID = message.readEnum16();
uint16_t eventActionDefinitionID = message.readEnum16();
if (eventActionDefinitionMap.find(eventDefinitionID) != eventActionDefinitionMap.end()) {
- bool eventActionDefinitionIDexists = false;
+ bool definitionIDexists = false;
auto range = eventActionDefinitionMap.equal_range(eventDefinitionID);
for (auto& element = range.first; element != range.second; ++element) {
- if (element->second.eventActionDefinitionID == eventActionDefinitionID){
+ if (element->second.eventActionDefinitionID == eventActionDefinitionID) {
element->second.enabled = true;
- eventActionDefinitionIDexists = true;
+ definitionIDexists = true;
+ break;
}
}
- if (eventActionDefinitionIDexists == false){
+ if (not definitionIDexists) {
ErrorHandler::reportError(message, ErrorHandler::EventActionUnknownEventActionDefinitionIDError);
}
} else {
@@ -111,24 +109,22 @@ void EventActionService::enableEventActionDefinitions(Message& message) {
void EventActionService::disableEventActionDefinitions(Message& message) {
// TC[19,5]
message.assertTC(19, 5);
- message.resetRead();
-
uint16_t numberOfEventActionDefinitions = message.readUint16();
if (numberOfEventActionDefinitions != 0) {
for (uint16_t i = 0; i < numberOfEventActionDefinitions; i++) {
- message.skipBytes(2);
+ message.skipBytes(2); // Skips reading applicationID
uint16_t eventDefinitionID = message.readEnum16();
uint16_t eventActionDefinitionID = message.readEnum16();
if (eventActionDefinitionMap.find(eventDefinitionID) != eventActionDefinitionMap.end()) {
- bool eventActionDefinitionIDexists = false;
+ bool definitionIDexists = false;
auto range = eventActionDefinitionMap.equal_range(eventDefinitionID);
for (auto& element = range.first; element != range.second; ++element) {
- if (element->second.eventActionDefinitionID == eventActionDefinitionID){
+ if (element->second.eventActionDefinitionID == eventActionDefinitionID) {
element->second.enabled = false;
- eventActionDefinitionIDexists = true;
+ definitionIDexists = true;
}
}
- if (eventActionDefinitionIDexists == false){
+ if (not definitionIDexists) {
ErrorHandler::reportError(message, ErrorHandler::EventActionUnknownEventActionDefinitionIDError);
}
} else {
@@ -181,14 +177,12 @@ void EventActionService::disableEventActionFunction(Message& message) {
void EventActionService::executeAction(uint16_t eventID) {
// Custom function
if (eventActionFunctionStatus) {
- if (eventActionDefinitionMap.find(eventID) != eventActionDefinitionMap.end()) {
- auto range = eventActionDefinitionMap.equal_range(eventID);
- for (auto& element = range.first; element != range.second; ++element){
- if (element->second.enabled == true){
- MessageParser messageParser;
- Message message = messageParser.parseRequestTC(element->second.request);
- messageParser.execute(message);
- }
+ auto range = eventActionDefinitionMap.equal_range(eventID);
+ for (auto& element = range.first; element != range.second; ++element) {
+ if (element->second.enabled) {
+ MessageParser messageParser;
+ Message message = messageParser.parseRequestTC(element->second.request);
+ messageParser.execute(message);
}
}
}
diff --git a/test/Services/EventActionService.cpp b/test/Services/EventActionService.cpp
index df396eea..1ba23a7c 100644
--- a/test/Services/EventActionService.cpp
+++ b/test/Services/EventActionService.cpp
@@ -53,7 +53,13 @@ TEST_CASE("Add event-action definitions TC[19,1]", "[service][st19]") {
CHECK(eventActionService.eventActionDefinitionMap.lower_bound(3)->second.request.compare(data) == 0);
// Adding the same message to check for error
- eventActionService.addEventActionDefinitions(message2);
+ Message message3(19, 1, Message::TC, 0);
+ message3.appendEnum16(1);
+ message3.appendEnum16(3);
+ message3.appendEnum16(1);
+ data = "456";
+ message3.appendString(data);
+ eventActionService.addEventActionDefinitions(message3);
CHECK(ServiceTests::thrownError(ErrorHandler::EventActionEventActionDefinitionIDExistsError));
CHECK(ServiceTests::countErrors() == 2);
}
--
GitLab