From 173a42f656e0943387c2b42310f0f82cb2a44e65 Mon Sep 17 00:00:00 2001
From: Ted John <ted@brambles.org>
Date: Wed, 26 Aug 2020 01:56:50 +0100
Subject: [PATCH] Apply code review suggestions

---
 src/openrct2/scripting/ScSocketServer.hpp | 57 +++++++++++++++--------
 src/openrct2/scripting/ScriptEngine.cpp   | 31 ++++++------
 src/openrct2/scripting/ScriptEngine.h     |  3 +-
 3 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/src/openrct2/scripting/ScSocketServer.hpp b/src/openrct2/scripting/ScSocketServer.hpp
index 98691b601a..b9c87c39aa 100644
--- a/src/openrct2/scripting/ScSocketServer.hpp
+++ b/src/openrct2/scripting/ScSocketServer.hpp
@@ -62,11 +62,7 @@ namespace OpenRCT2::Scripting
         void RemoveListener(uint32_t id, const DukValue& value)
         {
             auto& listeners = GetListenerList(id);
-            auto it = std::find(listeners.begin(), listeners.end(), value);
-            if (it != listeners.end())
-            {
-                listeners.erase(it);
-            }
+            listeners.erase(std::remove(listeners.begin(), listeners.end(), value), listeners.end());
         }
 
         void RemoveAllListeners(uint32_t id)
@@ -82,7 +78,7 @@ namespace OpenRCT2::Scripting
         std::shared_ptr<Plugin> _plugin;
 
     protected:
-        static bool IsLocalhostAddress(const std::string_view& s)
+        static bool IsLocalhostAddress(std::string_view s)
         {
             return s == "localhost" || s == "127.0.0.1" || s == "::";
         }
@@ -112,7 +108,7 @@ namespace OpenRCT2::Scripting
         virtual bool IsDisposed() const = 0;
     };
 
-    class ScSocket : public ScSocketBase
+    class ScSocket final : public ScSocketBase
     {
     private:
         static constexpr uint32_t EVENT_NONE = std::numeric_limits<uint32_t>::max();
@@ -199,8 +195,14 @@ namespace OpenRCT2::Scripting
                 if (data.type() == DukValue::Type::STRING)
                 {
                     write(data.as_string());
+                    _socket->Finish();
+                }
+                else
+                {
+                    _socket->Finish();
+                    auto ctx = GetContext()->GetScriptEngine().GetContext();
+                    duk_error(ctx, DUK_ERR_ERROR, "Only sending strings is currently supported.");
                 }
-                _socket->Finish();
             }
             return this;
         }
@@ -214,8 +216,15 @@ namespace OpenRCT2::Scripting
             }
             else if (_socket != nullptr)
             {
-                _socket->SendData(data.c_str(), data.size());
-                return true;
+                try
+                {
+                    auto sentBytes = _socket->SendData(data.c_str(), data.size());
+                    return sentBytes != data.size();
+                }
+                catch (const std::exception&)
+                {
+                    return false;
+                }
             }
             return false;
         }
@@ -263,7 +272,7 @@ namespace OpenRCT2::Scripting
             _eventList.Raise(EVENT_DATA, GetPlugin(), { ToDuk(ctx, data) }, false);
         }
 
-        uint32_t GetEventType(const std::string_view& name)
+        uint32_t GetEventType(std::string_view name)
         {
             if (name == "close")
                 return EVENT_CLOSE;
@@ -312,15 +321,13 @@ namespace OpenRCT2::Scripting
                         case NETWORK_READPACKET_MORE_DATA:
                             break;
                         case NETWORK_READPACKET_DISCONNECTED:
-                            CloseSocket();
-                            _disposed = true;
+                            Dispose();
                             break;
                     }
                 }
                 else
                 {
-                    CloseSocket();
-                    _disposed = true;
+                    Dispose();
                 }
             }
         }
@@ -348,7 +355,7 @@ namespace OpenRCT2::Scripting
         }
     };
 
-    class ScSocketServer : public ScSocketBase
+    class ScSocketServer final : public ScSocketBase
     {
     private:
         static constexpr uint32_t EVENT_NONE = std::numeric_limits<uint32_t>::max();
@@ -370,7 +377,7 @@ namespace OpenRCT2::Scripting
 
         ScSocketServer* close()
         {
-            Dispose();
+            CloseSocket();
             return this;
         }
 
@@ -399,7 +406,14 @@ namespace OpenRCT2::Scripting
                         auto host = dukHost.as_string();
                         if (IsLocalhostAddress(host))
                         {
-                            _socket->Listen(host, port);
+                            try
+                            {
+                                _socket->Listen(host, port);
+                            }
+                            catch (const std::exception& e)
+                            {
+                                duk_error(ctx, DUK_ERR_ERROR, e.what());
+                            }
                         }
                         else
                         {
@@ -475,13 +489,18 @@ namespace OpenRCT2::Scripting
             }
         }
 
-        void Dispose() override
+        void CloseSocket()
         {
             if (_socket != nullptr)
             {
                 _socket->Close();
                 _socket = nullptr;
             }
+        }
+
+        void Dispose() override
+        {
+            CloseSocket();
             _disposed = true;
         }
 
diff --git a/src/openrct2/scripting/ScriptEngine.cpp b/src/openrct2/scripting/ScriptEngine.cpp
index 3ba2bdcabc..f8b5639232 100644
--- a/src/openrct2/scripting/ScriptEngine.cpp
+++ b/src/openrct2/scripting/ScriptEngine.cpp
@@ -1145,13 +1145,18 @@ void ScriptEngine::UpdateSockets()
 {
 #    ifndef DISABLE_NETWORK
     // Use simple for i loop as Update calls can modify the list
-    for (size_t i = 0; i < _sockets.size(); i++)
+    auto it = _sockets.begin();
+    while (it != _sockets.end())
     {
-        _sockets[i]->Update();
-        if (_sockets[i]->IsDisposed())
+        auto& socket = *it;
+        socket->Update();
+        if (socket->IsDisposed())
         {
-            _sockets.erase(_sockets.begin() + i);
-            i--;
+            it = _sockets.erase(it);
+        }
+        else
+        {
+            it++;
         }
     }
 #    endif
@@ -1160,18 +1165,10 @@ void ScriptEngine::UpdateSockets()
 void ScriptEngine::RemoveSockets(const std::shared_ptr<Plugin>& plugin)
 {
 #    ifndef DISABLE_NETWORK
-    for (auto it = _sockets.begin(); it != _sockets.end();)
-    {
-        if ((*it)->GetPlugin() == plugin)
-        {
-            (*it)->Dispose();
-            it = _sockets.erase(it);
-        }
-        else
-        {
-            it++;
-        }
-    }
+    _sockets.erase(
+        std::remove_if(
+            _sockets.begin(), _sockets.end(), [&plugin](const auto& socket) { return socket->GetPlugin() == plugin; }),
+        _sockets.end());
 #    endif
 }
 
diff --git a/src/openrct2/scripting/ScriptEngine.h b/src/openrct2/scripting/ScriptEngine.h
index 6f08d2b4a0..c4de63bc56 100644
--- a/src/openrct2/scripting/ScriptEngine.h
+++ b/src/openrct2/scripting/ScriptEngine.h
@@ -19,6 +19,7 @@
 #    include "Plugin.h"
 
 #    include <future>
+#    include <list>
 #    include <memory>
 #    include <mutex>
 #    include <queue>
@@ -138,7 +139,7 @@ namespace OpenRCT2::Scripting
 
         std::unordered_map<std::string, CustomActionInfo> _customActions;
 #    ifndef DISABLE_NETWORK
-        std::vector<std::shared_ptr<ScSocketBase>> _sockets;
+        std::list<std::shared_ptr<ScSocketBase>> _sockets;
 #    endif
 
     public: