Code review comments - added constness, fixed if check on Attachable, wrote out autos that were used for iterators, fixed range loops not using const references

Author: garethyr

Entities/Actor.h

@@ -1538,7 +1538,7 @@ ENTITYALLOCATION(Actor)
     /// Gets a vector containing the script function names this class supports.
     /// </summary>
     /// <returns>A vector containing the script function names this class supports.</returns>
-    virtual const std::vector<std::string> GetSupportedScriptFunctionNames() override { auto functionNames = MOSRotating::GetSupportedScriptFunctionNames(); functionNames.insert(functionNames.end(), {"UpdateAI"}); return functionNames; }
+    virtual const std::vector<std::string> GetSupportedScriptFunctionNames() const override { auto functionNames = MOSRotating::GetSupportedScriptFunctionNames(); functionNames.insert(functionNames.end(), {"UpdateAI"}); return functionNames; }
 
 //////////////////////////////////////////////////////////////////////////////////////////
 // Private member variable and method declarations

Entities/Attachable.cpp

@@ -306,7 +306,7 @@ void Attachable::Detach()
 
     m_RestTimer.Reset();
 
-    if (temporaryParent != NULL) {
+    if (temporaryParent != NULL && temporaryParent->ObjectScriptsInitialized()) {
         RunScriptedFunctionInAppropriateScripts("OnDetach", false, false, {temporaryParent});
     }
 }

Entities/Attachable.h

@@ -704,7 +704,7 @@ ENTITYALLOCATION(Attachable)
     /// Gets a vector containing the script function names this class supports.
     /// </summary>
     /// <returns>A vector containing the script function names this class supports.</returns>
-    virtual const std::vector<std::string> GetSupportedScriptFunctionNames() override { auto functionNames = MOSRotating::GetSupportedScriptFunctionNames(); functionNames.insert(functionNames.end(), {"OnAttach", "OnDetach"}); return functionNames; }
+    virtual const std::vector<std::string> GetSupportedScriptFunctionNames() const override { auto functionNames = MOSRotating::GetSupportedScriptFunctionNames(); functionNames.insert(functionNames.end(), {"OnAttach", "OnDetach"}); return functionNames; }
 
 
 //////////////////////////////////////////////////////////////////////////////////////////

Entities/MovableObject.cpp

@@ -205,7 +205,7 @@ int MovableObject::Create(const MovableObject &reference)
     m_MissionCritical = reference.m_MissionCritical;
     m_CanBeSquished = reference.m_CanBeSquished;
     m_HUDVisible = reference.m_HUDVisible;
-    for (std::pair<std::string, bool> scriptEntry : reference.m_LoadedScripts) {
+    for (const std::pair<std::string, bool> &scriptEntry : reference.m_LoadedScripts) {
         m_LoadedScripts.push_back({scriptEntry.first, scriptEntry.second});
     }
     m_ScriptPresetName = reference.m_ScriptPresetName;
@@ -329,9 +329,7 @@ int MovableObject::ReadProperty(std::string propName, Reader &reader)
 	else if (propName == "ScriptPath")
     {
         std::string scriptPath = reader.ReadPropValue();
-        if (LoadScript(scriptPath) == -2) {
-            reader.ReportError("Duplicate script path " + scriptPath);
-        }
+        if (LoadScript(scriptPath) == -2) { reader.ReportError("Duplicate script path " + scriptPath); }
     }
     else if (propName == "ScreenEffect")
     {
@@ -495,7 +493,7 @@ int MovableObject::LoadScript(const std::string &scriptPath, bool loadAsEnabledS
     m_LoadedScripts.push_back({scriptPath, loadAsEnabledScript});
 
     // Clear the temporary variable names that will hold the functions read in from the file
-    for (std::string functionName : GetSupportedScriptFunctionNames()) {
+    for (const std::string &functionName : GetSupportedScriptFunctionNames()) {
         if (g_LuaMan.RunScriptString(functionName + " = nil;") < 0) {
             return -3;
         }
@@ -551,7 +549,7 @@ int MovableObject::ReloadScripts() {
         object->m_ScriptPresetName.clear();
 
         int status = 0;
-        for (std::pair<std::string, bool> scriptEntry : loadedScriptsCopy) {
+        for (const std::pair<std::string, bool> &scriptEntry : loadedScriptsCopy) {
             status = object->LoadScript(scriptEntry.first, scriptEntry.second);
             if (status < 0) {
                 return status;
@@ -606,7 +604,7 @@ bool MovableObject::RemoveScript(const std::string &scriptPath) {
         return false;
     }
 
-    auto scriptEntryIterator = FindScript(scriptPath);
+    std::vector<std::pair<std::string, bool>>::const_iterator scriptEntryIterator = FindScript(scriptPath);
     if (scriptEntryIterator != m_LoadedScripts.end()) {
         m_LoadedScripts.erase(scriptEntryIterator);
         if (ObjectScriptsInitialized() && RunScriptedFunction(scriptPath, "OnScriptRemoveOrDisable", {}, {"true"}) < 0) {
@@ -625,7 +623,7 @@ bool MovableObject::EnableScript(const std::string &scriptPath) {
         return false;
     }
 
-    auto scriptEntryIterator = FindScript(scriptPath);
+    std::vector<std::pair<std::string, bool>>::iterator scriptEntryIterator = FindScript(scriptPath);
     if (scriptEntryIterator != m_LoadedScripts.end() && scriptEntryIterator->second == false) {
         if (ObjectScriptsInitialized() && RunScriptedFunction(scriptPath, "OnScriptEnable") < 0) {
             return false;
@@ -643,7 +641,7 @@ bool MovableObject::DisableScript(const std::string &scriptPath) {
         return false;
     }
 
-    auto scriptEntryIterator = FindScript(scriptPath);
+    std::vector<std::pair<std::string, bool>>::iterator scriptEntryIterator = FindScript(scriptPath);
     if (scriptEntryIterator != m_LoadedScripts.end() && scriptEntryIterator->second == true) {
         if (ObjectScriptsInitialized() && RunScriptedFunction(scriptPath, "OnScriptRemoveOrDisable", {}, {"false"}) < 0) {
             return false;
@@ -684,7 +682,7 @@ int MovableObject::RunScriptedFunctionInAppropriateScripts(const std::string &fu
     }
 
     int status = 0;
-    for (std::pair<std::string, bool> scriptEntry : m_LoadedScripts) {
+    for (const std::pair<std::string, bool> &scriptEntry : m_LoadedScripts) {
         if (runOnDisabledScripts || scriptEntry.second == true) {
             status = RunScriptedFunction(scriptEntry.first, functionName, functionEntityArguments, functionLiteralArguments);
             if (status < 0 && stopOnError) {

Entities/MovableObject.h

@@ -199,12 +199,19 @@ ENTITYALLOCATION(MovableObject)
     /// <returns>The iterator pointing to the vector entry for the script or the end of the vector if the script was not found.</returns>
     virtual std::vector<std::pair<std::string, bool>>::iterator const FindScript(std::string const &scriptPath) { return std::find_if(m_LoadedScripts.begin(), m_LoadedScripts.end(), [&scriptPath](auto element) { return element.first == scriptPath; }); }
 
+    /// <summary>
+    /// Convenience method to get the script at the given path if it's on this MO. Like standard find, returns m_LoadedScripts.cend() if it's not.
+    /// </summary>
+    /// <param name="scriptPath">The path to the script to find.</param>
+    /// <returns>The iterator pointing to the vector entry for the script or the end of the vector if the script was not found.</returns>
+    virtual std::vector<std::pair<std::string, bool>>::const_iterator const FindScript(std::string const &scriptPath) const { return std::find_if(m_LoadedScripts.cbegin(), m_LoadedScripts.cend(), [&scriptPath](auto element) { return element.first == scriptPath; }); }
+
     /// <summary>
     /// Checks if the script at the given path is one of the scripts on this MO.
     /// </summary>
     /// <param name="scriptPath">The path to the script to check.</param>
     /// <returns>Whether or not the script is on this MO.</returns>
-    virtual bool const HasScript(const std::string &scriptPath) { return FindScript(scriptPath) != m_LoadedScripts.end(); }
+    virtual bool const HasScript(const std::string &scriptPath) const { return FindScript(scriptPath) != m_LoadedScripts.end(); }
 
     /// <summary>
     /// Adds the script at the given path as one of the scripts on this MO.
@@ -225,7 +232,7 @@ ENTITYALLOCATION(MovableObject)
     /// </summary>
     /// <param name="scriptPath">The path to the script to check.</param>
     /// <returns>Whether or not the script is enabled on this MO.</returns>
-    virtual bool const ScriptEnabled(const std::string &scriptPath) { auto scriptIterator = FindScript(scriptPath); return scriptIterator != m_LoadedScripts.end() && scriptIterator->second == true; }
+    virtual bool const ScriptEnabled(const std::string &scriptPath) const { auto scriptIterator = FindScript(scriptPath); return scriptIterator != m_LoadedScripts.end() && scriptIterator->second == true; }
 
     /// <summary>
     /// Enable the script at the given path on this MO.
@@ -268,7 +275,7 @@ ENTITYALLOCATION(MovableObject)
     /// Gets whether or not the object has a script name, and there were no errors when initializing its Lua scripts. If there were, the object would need to be reloaded.
     /// </summary>
     /// <returns>Whether or not the object's scripts have been succesfully initialized.</returns>
-    bool ObjectScriptsInitialized() { return !m_ScriptObjectName.empty() && m_ScriptObjectName != "ERROR"; }
+    bool ObjectScriptsInitialized() const { return !m_ScriptObjectName.empty() && m_ScriptObjectName != "ERROR"; }
 
 //////////////////////////////////////////////////////////////////////////////////////////
 // Virtual method:  GetClass
@@ -1776,7 +1783,7 @@ ENTITYALLOCATION(MovableObject)
     /// Gets a vector containing the script function names this class supports.
     /// </summary>
     /// <returns>A vector containing the script function names this class supports.</returns>
-    virtual const std::vector<std::string> GetSupportedScriptFunctionNames() { return std::vector<std::string> {"Create", "Destroy", "Update", "OnScriptRemoveOrDisable", "OnScriptEnable", "OnPieMenu"}; }
+    virtual const std::vector<std::string> GetSupportedScriptFunctionNames() const { return std::vector<std::string> {"Create", "Destroy", "Update", "OnScriptRemoveOrDisable", "OnScriptEnable", "OnPieMenu", "OnCollideWithTerrain", "OnCollideWithMO"}; }
 
     /// <summary>
     /// Does necessary work to setup a script object name for this object, allowing it to be accessed in Lua, then runs all of the MO's scripts' Create functions in Lua.