Correct comment typos and documentation

Also, add unit tests for Extract along with updating the related documentation

ACore/CMakeLists.txt

@@ -131,6 +131,7 @@ set(test_srcs
   test/TestClassDataMemberInit.cpp
   test/TestCommandLine.cpp
   test/TestCore_main.cpp # contains main() function for test driver
+  test/TestExtract.cpp
   test/TestFile.cpp
   test/TestGetUserDetails.cpp
   test/TestLog.cpp

ACore/src/ecflow/core/Calendar.hpp

@@ -218,7 +218,7 @@ class Calendar {
 
 private:
     // Note: The *only* reason to serialise the calendar is so that we can support
-    // why() command on the client side. By default calendar is initialised in the *server*
+    // why() command on the client side. By default, calendar is initialised in the *server*
     // at begin time, from the clock attribute
     friend class cereal::access;
     template <class Archive>

ACore/src/ecflow/core/EcfPortLock.hpp

@@ -42,8 +42,6 @@ class EcfPortLock {
 
     static void create(const std::string& the_port) {
         std::string the_file = port_file(the_port);
-        //      std::cout << "EcfPortLock::create " << the_file << "
-        //      ---------------------------------------------------\n";
         std::string errorMsg;
         if (!ecf::File::create(the_file, "", errorMsg)) {
             std::stringstream sb;
@@ -54,8 +52,6 @@ class EcfPortLock {
 
     static void remove(const std::string& the_port) {
         std::string the_file = port_file(the_port);
-        //      std::cout << "EcfPortLock::remove " << the_file << "
-        //      --------------------------------------------------\n";
         fs::remove(the_file);
     }
 

ACore/src/ecflow/core/Extract.cpp

@@ -29,11 +29,6 @@ ostream& operator<<(ostream& os, const vector<T>& v) {
 }
 
 bool Extract::pathAndName(const std::string& token, std::string& path, std::string& name) {
-    // cout << "Extract::pathAndName  token = " << token << "\n";
-    // can have:
-    //            /suite/family:obj      path = /suite/family   name = obj
-    //            /suite/family          path = /suite/family   name =
-    //            obj                    path =                 name = obj
     if (token.empty())
         return false;
 
@@ -51,7 +46,6 @@ bool Extract::pathAndName(const std::string& token, std::string& path, std::stri
         name = token.substr(colonPos + 1);
     }
 
-    //	cout << "Extract::pathAndName token=" << token << " path= '" << path << "' name= '" << name << "'\n";
     return true;
 }
 
@@ -95,22 +89,14 @@ int Extract::ymd(const std::string& ymdToken, std::string& errorMsg) {
     return theInt(ymdToken, errorMsg);
 }
 
-int Extract::optionalInt(const std::vector<std::string>& lineTokens,
+int Extract::optionalInt(const std::vector<std::string>& tokens,
                          int pos,
                          int defaultValue,
                          const std::string& errorMsg) {
-    // token0 token1 token2 token3   size = 4
-    // pos0   pos1   pos2   pos3
-    //
-    // could have line of the form
-    // repeat integer variable 1 2 #a comment
-    // repeat integer variable 3 4 # a comment
-    // hence we must check the first character, and not the complete token
-
     int the_int = defaultValue;
-    if (static_cast<int>(lineTokens.size()) >= pos + 1 && lineTokens[pos][0] != '#') {
+    if (static_cast<int>(tokens.size()) >= pos + 1 && tokens[pos][0] != '#') {
 
-        the_int = theInt(lineTokens[pos], errorMsg);
+        the_int = theInt(tokens[pos], errorMsg);
     }
     return the_int;
 }

ACore/src/ecflow/core/Extract.hpp

@@ -16,33 +16,62 @@
 
 class Extract {
 public:
-    // token if of the form:
-    //    /path/to/home:obj        OR    obj
-    //    path = /path/to/home           path = ""
-    //    name = obj                     name = obj
-    // return true for ok or false for error
+    ///
+    /// Extract path and name from given token.
+    ///
+    /// Given
+    ///   "/suite/family:obj", extracts path = "/suite/family", and name = "obj"
+    ///   "/suite/family", extracts path = "/suite/family", and name = ""
+    ///   "obj", extracts path = "", and name = "obj"
+    /// @returns true if extraction succeeded; false, otherwise
+    ///
     static bool pathAndName(const std::string& token, std::string& path, std::string& name);
 
-    // Given str = HH:MM
-    // return MM;
+    ///
+    /// Extract 2nd token from the given string (considering provided separator)
+    ///
+    /// Given str = "HH:MM", extracts ret = "MM"
+    ///
+    /// @returns true if extraction succeeded; false, otherwise
+    ///
     static bool split_get_second(const std::string& str, std::string& ret, char separator = ':');
 
-    /// extract integer or throw an std::runtime_error on failure,
-    /// the error message passed in is used to configure the returned exception
+    ///
+    /// Extract an integer from the given token
+    ///
+    /// @returns the extracted integer
+    /// @throws std::runtime_error if extractions fails, with the provided error message included in exception message
+    ///
     static int theInt(const std::string& token, const std::string& errorMsg);
 
-    /// extract YMD, integer of the form yyyymmdd, will throw std::runtime_error on failure
-    /// the error message passed in is used to configure the returned exception
+    ///
+    /// Extract YMD integer, of the form yyyymmdd, from the given token
+    ///
+    /// @throws std::runtime_error if extractions fails, with the provided error message included in exception message
+    ///
     static int ymd(const std::string& ymdToken, std::string& errorMsg);
 
-    /// extract optional int, else return -1
-    /// the error message passed in is used to configure the returned exception
-    // could have line of the form
-    // repeat integer variable 1 2 #a comment
-    // repeat integer variable 3 4 # a comment
-    // hence we must check the first character, and not the complete token
-    static int
-    optionalInt(const std::vector<std::string>& lineTokens, int pos, int defaultValue, const std::string& errorMsg);
+    ///
+    /// Extract (optional) integer, from token in index pos
+    ///
+    /// Notice: when first character of selected token is '#', the return is the provided default value
+    ///
+    /// Example of tokens:
+    ///   ["repeat", "integer", "variable", "1", "2", "#a", "comment"]
+    ///    - extracting from "repeat", "integer", "variable", or "comment" throws std::runtime_error
+    ///    - extracting from "1", returns 1
+    ///    - extracting from "2", returns 3
+    ///    - extracting from "#a", returns the given default value
+    ///   ["repeat", "integer", "variable", "1", "2", "#", "a", "comment"]
+    ///    - extracting from "repeat", "integer", "variable", "a", or "comment" throws std::runtime_error
+    ///    - extracting from "1", returns 1
+    ///    - extracting from "2", returns 3
+    ///    - extracting from "#", returns the given default value
+    ///
+    /// @returns the extracted integer, if extraction succeeded; default value if selected token starts with '#'
+    /// @throws std::runtime_error if extractions fails, with the provided error message included in exception message
+    ///
+    static int optionalInt(const std::vector<std::string>& tokens, int pos, int defValue, const std::string& errorMsg);
 
 private:
     Extract()                                = delete;

ACore/src/ecflow/core/File.cpp

@@ -152,7 +152,7 @@ bool File::splitFileIntoLines(const std::string& filename, std::vector<std::stri
     //	tokenizer tokens(file_iter,end_of_stream, sep);
     // std::copy(tokens.begin(), tokens.end(), back_inserter(lines));
 
-    // The current implementation is 2.5 times faster then method 1, and ~4 times faster than method 2
+    // The current implementation is 2.5 times faster than method 1, and ~4 times faster than method 2
     return true;
 }
 
@@ -239,7 +239,7 @@ bool File::open(const std::string& filePath, std::string& contents) {
 }
 
 bool File::create(const std::string& filename, const std::vector<std::string>& lines, std::string& errorMsg) {
-    // For very large file. This is about 1 second quicker. Than using streams
+    // For very large file. This is about 1 second quicker. Then using streams
     // See Test: TestFile.cpp:test_file_create_perf
     FILE* theFile = fopen(filename.c_str(), "w");
     if (theFile == nullptr) {
@@ -504,7 +504,7 @@ bool File::createMissingDirectories(const std::string& pathToFileOrDir) {
             // pathToFileOrDir is of form: /tmp/fred/sms.job
             //   we should only create directories for /tmp/fred
             if (thePath.back().find(".") != std::string::npos) {
-                // assume the last token represents a file, hence dont create a directory
+                // assume the last token represents a file, hence don't create a directory
 #ifdef INTEL_DEBUG_ME
                 std::cout << "   last file " << thePath.back() << " has a *dot* ignoring " << std::endl;
 #endif
@@ -650,7 +650,7 @@ std::string File::diff(const std::string& file,
 std::string
 File::backwardSearch(const std::string& rootPath, const std::string& nodePath, const std::string& fileExtn) {
     // Do a backward search of rootPath + nodePath
-    // If task path if of the form /suite/family/family2/task, then we keep
+    // If task path is of the form /suite/family/family2/task, then we keep
     // on consuming the first path token this should leave:
     //   	<root-path>/suite/family/family2/task.ecf
     //   	<root-path>/family/family2/task.ecf

ACore/src/ecflow/core/Host.hpp

@@ -51,7 +51,7 @@ class Host {
 
 private:
     std::string host_port_prefix(const std::string& port) const;
-    void get_host_name(); // will cache host name, to avoid multiple sysm calls
+    void get_host_name(); // will cache host name, to avoid multiple system calls
     std::string the_host_name_;
 };
 

ACore/src/ecflow/core/LogVerification.cpp

@@ -100,11 +100,11 @@ bool LogVerification::compareNodeStates(const std::string& logfile,
             if (i < lines.size() && i < goldenLines.size()) {
 
                 if (lines[i] != goldenLines[i]) {
-                    // Please note that we can't do an exact compare for certain state changes
-                    // 	 	submitted -->active
-                    //  	active    -->complete
+                    // Please note that we can't do an exact comparison for certain state changes:
+                    //   - submitted --> active
+                    //   - active    --> complete
                     // Since this can be OS/scheduler dependent and hence order dependent
-                    // To compensate look search Golden file
+                    // To compensate, we search for a Golden file
                     if (lines[i].second == "submitted" || lines[i].second == "active" ||
                         lines[i].second == "complete") {
                         // search for line in golden file

ACore/src/ecflow/core/NState.hpp

@@ -20,10 +20,10 @@ class access;
 
 // NState: stores the state of a node.
 // *The class NState just used to define the enum, however we also
-// needed to know when the state changed. Hence the use of state_change_no
+// needed to know when the state changed. Hence, the use of state_change_no
 // Uses default copy constructor and destructor, and equality
 //
-// The default_state() should *NEVER* change as it will affect client/server comms
+// The default_state() should *NEVER* change as it will impact client/server communications
 //
 class NState {
 public:

ACore/src/ecflow/core/PasswdFile.cpp

@@ -319,7 +319,7 @@ bool PasswdFile::createWithAccess(const std::string& pathToFile,
 bool PasswdFile::clear(const std::string& pathToFile, std::string& errorMsg) {
     std::vector<std::string> lines;
     if (File::splitFileIntoLines(pathToFile, lines, true /* ignore empty lines */)) {
-        // Just leave the version. i.e the first line.
+        // Just leave the version, i.e. the first line.
         if (lines.size() > 1) {
             lines.erase(lines.begin() + 1, lines.end());
 

ACore/src/ecflow/core/PasswdFile.hpp

@@ -41,10 +41,10 @@ class Pass_wd {
     std::string user_;
     std::string host_;
     std::string port_;
-    std::string passwd_; // always stored as crypted
+    std::string passwd_; // always stored as ciphertext
 };
 
-// This class is used to authenticate, user commands, i.e like ping,alter, etc
+// This class is used to authenticate, user commands, i.e. like ping,alter, etc
 class PasswdFile {
 public:
     PasswdFile();
@@ -59,7 +59,7 @@ class PasswdFile {
     // to be called by the server, to at least one user with given host and port
     bool check_at_least_one_user_with_host_and_port(const std::string& host, const std::string& port);
 
-    // get the password for the given user, host and port. Otherwise return a empty string
+    // get the password for the given user, host and port. Otherwise, return an empty string
     std::string get_passwd(const std::string& user, const std::string& host, const std::string& port);
 
     // authenticate the user, given the password.

ACore/src/ecflow/core/Pid.hpp

@@ -15,8 +15,7 @@
 
 class Pid {
 public:
-    /// Returns the current Process ID (as a string);
-    /// otherwise, throws exception(std::runtime_error)
+    /// Returns the current Process ID (as a string); otherwise, throws exception(std::runtime_error)
     static std::string getpid();
 
     /// Returns a unique name, based on Process ID, composed of prefix + '_' + getpid();

ACore/src/ecflow/core/PrintStyle.cpp

@@ -17,8 +17,6 @@ PrintStyle::Type_t PrintStyle::getStyle() {
 }
 
 void PrintStyle::setStyle(PrintStyle::Type_t f) {
-    // std::cout << "PrintStyle::setStyle() BEFORE " << PrintStyle::to_string(style_) << " AFTER " <<
-    // PrintStyle::to_string(f) << "   *********************\n";
     style_ = f;
 }
 

ACore/src/ecflow/core/PrintStyle.hpp

@@ -16,9 +16,9 @@
 class PrintStyle {
 public:
     // Use to control the print defs/node output to string or file
-    // Please note: with PrintStyle::NET is used to transfer Defs between client <->server
+    // Please note: with PrintStyle::NET is used to transfer Defs between client <-> server
     //              as such there is no need extensive checking on recreating defs.
-    //              i.e valid name, duplicate nodes, etc
+    //              i.e. valid name, duplicate nodes, etc
     enum Type_t {
         // Does nothing
         NOTHING = 0,

ACore/src/ecflow/core/SerializationTest.hpp

@@ -28,7 +28,6 @@ void do_restore(const std::string& fileName, const T& saved) {
         BOOST_CHECK_MESSAGE(false, "Restore failed because: " << e.what());
     }
     BOOST_CHECK_MESSAGE(saved == restored, "save and restored don't match for " << fileName << "\n");
-    //   BOOST_CHECK_MESSAGE(saved == restored," save and restored don't match\n" << saved  << "\n" << restored );
 }
 
 template <typename T>

ACore/src/ecflow/core/Str.cpp

@@ -214,10 +214,10 @@ void Str::removeSingleQuotes(std::string& s) {
     }
 }
 
-bool Str::replace(std::string& jobLine, const std::string& stringToFind, const std::string& stringToRplace) {
+bool Str::replace(std::string& jobLine, const std::string& stringToFind, const std::string& stringToReplace) {
     size_t pos = jobLine.find(stringToFind);
     if (pos != string::npos) {
-        jobLine.replace(pos, stringToFind.length(), stringToRplace);
+        jobLine.replace(pos, stringToFind.length(), stringToReplace);
         return true;
     }
     return false;

ACore/src/ecflow/core/Str.hpp

@@ -68,11 +68,11 @@ class Str {
     //  fred  -> fred
     static void removeSingleQuotes(std::string&);
 
-    /// Find 'stringToFind' in 'jobLine' and replace with string 'stringToRplace'
+    /// Find 'stringToFind' in 'jobLine' and replace with string 'stringToReplace'
     /// return true if replace ok else returns false;
-    static bool replace(std::string& subject, const std::string& stringToFind, const std::string& stringToRplace);
-    static bool replace_all(std::string& subject, const std::string& stringToFind, const std::string& stringToRplace);
-    static void replaceall(std::string& subject, const std::string& stringToFind, const std::string& stringToRplace);
+    static bool replace(std::string& subject, const std::string& stringToFind, const std::string& stringToReplace);
+    static bool replace_all(std::string& subject, const std::string& stringToFind, const std::string& stringToReplace);
+    static void replaceall(std::string& subject, const std::string& stringToFind, const std::string& stringToReplace);
 
     // extract data member value, ie given a string of the form:
     //   str=cmd a b fred:value
@@ -130,10 +130,10 @@ class Str {
     /// case in sensitive string comparison
     static bool caseInsCompare(const std::string&, const std::string&);
 
-    /// case insenstive less
+    /// case insensitive less
     static bool caseInsLess(const std::string&, const std::string&);
 
-    /// case insenstive Greater
+    /// case insensitive Greater
     static bool caseInsGreater(const std::string&, const std::string&);
 
     /// Used for checking node names
@@ -159,7 +159,7 @@ class Str {
     /// Only use strcmp if the first characters are the same
     static int local_strcmp(const char* s, const char* t) { return (*s != *t ? *s - *t : strcmp(s, t)); }
 
-    // returns a static string of alpha numerics and underscore
+    // returns a static string of alphanumerics and underscore
     static const std::string& ALPHANUMERIC_UNDERSCORE();
 
     // returns a static string of numerics chars

ACore/src/ecflow/core/StringSplitter.cpp

@@ -57,15 +57,16 @@ void StringSplitter::split(const std::string& str,
     }
 }
 
-void StringSplitter::split2(std::string_view str, std::vector<std::string_view>& ret, const char* delims) {
+void StringSplitter::split2(std::string_view str, std::vector<std::string_view>& ret, const char* delimiters) {
     std::string_view::size_type start = 0;
-    auto pos                          = str.find_first_of(delims, start);
+
+    auto pos = str.find_first_of(delimiters, start);
     while (pos != std::string_view::npos) {
         if (pos != start) {
             ret.emplace_back(str.substr(start, pos - start));
         }
         start = pos + 1;
-        pos   = str.find_first_of(delims, start);
+        pos   = str.find_first_of(delimiters, start);
     }
     if (start < str.length())
         ret.emplace_back(str.substr(start, str.length() - start));

ACore/src/ecflow/core/StringSplitter.hpp

@@ -22,7 +22,7 @@ namespace ecf {
 // IMPORTANT: std::string_view is a *READ ONLY* REFERENCE to an existing string
 // HENCE:     the reference string *MUST* not change, and its lifetime must EXCEED string_view
 //
-// Will split a string. Will return a empty std::string_view if there a separator at the end.
+// Will split a string. Will return an empty std::string_view if there's a separator at the end.
 // This shows the fastest split for a string. **** Based on boost 1.64 ****
 //    Method:              time
 //    boost::split:        4.06