Code Improvements (WIP)

[ryantaplin]

Changes & Improvements PR

WORK IN PROGRESS

DISCLAIMER

This PR is a work in progress it is by no means finished yet. I will aim to always push my changes in a working state so that it's readily available from a merging perspective if at any point you wish to adopt these changes. A running change log is being posted when possible. Please review all changes carefully.

Context

This PR is here to raise awareness of the changes i'm making and to share the time and effort i'm actively putting into the source code right now. These improvements will hopefully make the code more clean, readable and consistent using industry standard coding practices & principles like SOLID and DRY. There is by NO means any expectation or commitment for this to be accepted by the community & maintainers of this source.

Any questions, comments or suggestions please feel free to post below. Please refrain from posting anything unrelated to the goal in this PR. I will not be taking on feature requests here.

High Level TLDR

• Ownership and management of TcpListener moved from SEnvir into dedicated TcpServer.
  • TcpListener functions moved out of SEnvir and into dedicated IListenerHandler implementations based on their individual responsibility.
• Ownership and management of TcpClient // UserConnection previously known as SConnection moved out of SEnvir and into UserConnectionService. Several utility functions added to support this transition including a dedicated IpAddrManagementService, UserConnectionFactory, UserBroadcastService.

LowLevel TLDR

• TcpServer now encapsulates and handles lifecycle of TcpListener.
• IListenerHandler introduced to allow for extensible, single responsibility TcpListener behaviours. ListenerHandler currently encapsulates a number of the existing Listeners lifecycle state. There are two implementations of this to maintain existing functionality.
  • ActiveUserCountListenerHandler to support existing LOMCN UserCount checking.
  • UserConnectionListenerHandler to support existing new user connections (i.e connects to the server and logs in).
• UserConnectionService (including abstract) introduced to encapsulate the management and lifecycle of UserConnection (previously known as SConnections).
  • Note: There may be room for improvement on some of this implementation, for example UserConnectionFactory : AbstractConnectionFactory was overengineered with the intention to re-use the Abstract on UserCount (which later was deemed unnecessary.
    • IConnectionFactory only exists as a utility to Abstract implementation.
• IpAddressService encapsulates and handles management of IpAddress, specifically IP specific timeouts behaviours, with the intent to possibly extend this in future with other functionality (i.e Ip throttling).
• UserBroadcastService exists purely as a way to bridge some gaps between user chat broadcasting functionality. This may not be the final solution for this functionality, a review of this implementation will come later.

📦 ServerLibrary
└─ Infrastructure
   ├─ Network
   │  ├─ Tcp
   │  │  ├─ ListenerHandler
   │  │  │  ├─ IListenerHandler.class
   │  │  │  ├─ ActiveUserCountListenerHandler.class
   │  │  │  └─ UserConnectionListenerHandler.class
   │  │  └─ TcpServer.class
   │  ├─ Smtp*
   │  │  └─ EmailService.class
   │  └─ Http*
   │    └─ HttpWebServer.class
   └─ Service
      ├─ Connection
      │  ├─ AbstractConnectionService.class
      │  ├─ IConnectionFactory.class
      │  ├─ UserConnection.class
      │  ├─ UserConnectionFactory.class
      │  └─ UserConnectionService.class
      ├─ IpAddrService.class
      └─ UserBroadcastingService.class

* Smtp and Http packages were eagerly introduced for 
EmailService and WebServer which will eventually follow 
the same refactoring / improvements. No underlying 
changes were made to these classes.

Further Considerations:

• Simplify ConnectionService by removing Abstract & dependency on Factory as per previous notes.
• Introduce dependency injection either manually or using a framework (i.e AutoFac). to move heavy reliance on static reference across the codebase (including SEnvir). The goal is to eventually move everything away from static reference in this changeset.

[Lilcooldoode]

Caution: This guy has a known history of intentionally adding malicious code into Mir3 projects. Review all changes with heightened scrutiny.

[ryantaplin]

Thanks for your input lil dude.

Please review to your hearts desire, must be a lot of malicious code in the tcp logic.. 😄

[ryantaplin]

Latest Changes & Improvements: Logging

NOTE: All existing user-facing functionality should be unchanged. If something looks off please let me know and i'll look into it.

High Level TLDR:

• SEnvir.Log() replaced with SEnvir.SystemLogger.Log()
• SEnvir.ChatLog replaced with SEnvir.UserChatLogger.Log()
• WriteLogsLoop no longer has Chat/System log coupling. Each associated logger is now responsible for their own file writing schedule.

Low Level TLDR:

• ILogger introduced to allow extensible, single responsibility Logger behaviours. There are now several implementations of ILogger based on individual functions.
  • CompositeLogger to encapsulate handling and support of having 1:N loggers.
  • SystemAppLogger to encapsulate writing system logs to the ServerGUI.
    • Note: the ConcurrentQueue<string> ServerAppLogs previously known as ServerDisplayLogs is still owned by SEnvir to maintain changeset. Eventually Server module should inject ServerAppLogger and maintain its own reference to this queue or directly mutate the BindingList<string> Logs reference in the server module.
  • SystemConsoleLogger to encapsulate writing system logs to console.
  • SystemFileLogger to encapsulate writing system logs to file.
  • UserChatAppLogger to encapsulate writing logs to the ServerGUI
    • Note: the ConcurrentQueue<string> ChatAppLogs previously known as ChatDisplayLogs is still owned by SEnvir to maintain changeset. Eventually Server module should inject ChatAppLogger and maintain its own reference to this queue or directly mutate the BindingList<string> Logs reference in the server module.
  • UserChatFileLogger to encapsulate writing chat logs to file.
• ILogFormatter to allow extensible log formatting behaviours. Only one implementation of this exists i.e StdLogFormatter which maintains existing behaviour.
• SingleThreadScheduler introduced to allow tasks to be schedule on a shared thread. This was done to retain the same behaviour previously exhibited in WriteLogsLoop and as a consideration for thread resource efficiency.

📦 Server Library
└─ Infrastructure
   ├─ Logging
   │  ├─ Formatter
   │  │  ├─ ILogFormatter.cs
   │  │  └─ StdLogFormatter.cs
   │  ├─ CompositeLogger.cs
   │  ├─ SystemAppLogger.cs
   │  ├─ SystemConsoleLogger.cs
   │  ├─ SystemFileLogger.cs
   │  ├─ UserChatAppLogger.cs
   │  └─ UserChatFileLogger.cs
   └─ Scheduler
      └─ SingleThreadScheduler.cs

Further considerations:

• SEnvir should not own the logic behind instantiating the ILogger implementations. Ideally SEnvir.SystemLogger and SEnvir.UserChatLogger should be injected by the relevant collaborators (i.e Server & ServerCore). This would also mean UseLogConsole would become redundant and can be removed.
• No changes where made to how Server module schedules / handles the display of logs in the UI (this is still controlled as before using InterfaceTimer_Tick function). This could be brought inline with other logger implementations if the Server module was to take ownership of the SystemAppLogger implementation and inject into the ServerLibrary module.

[ryantaplin]

For disclosure I had a couple of attempts at refactoring the DB logic, moving its distributed logic & responsibility to a repository style pattern.

Unfortunately taking the minimalistic refactoring approach with the DB logic rendered a convoluted and complex outcome I wasn't happy with so I've ditched those changes and decided to think about the components and their responsibilities a bit more before I attempt to take that challenge on again. It may be the case that a slight overhaul of those components is necessary.

Initial Analysis

• Session loads & saves (User & System) data. Data is saved onto filesystem, persisted as binary.
  • Save() / Commit() is done in a two phase approach (1. persist state of DbObject as binary to memory via RawData) 2. Write RawData to file system). Not sure what for what reason we need to hold the binary representation in memory indefinitely - it just concerns me that we're holding all this data in memory permenantly.
  • Session contains logic for both system & user data. These should ideally be separated into their own sub-components.
• DBCollection / ADBCollection manages List including loading (via Session), retrieval, creation (with indexing), saving (via Session) and deletion
  • Has a bespoke searching algorithm for deletion?
• DBObject hold and manages specific state (ItemInfo, CharacterInfo etc), including saving, loading and deleting & linking (tbc).
• DBMapping I believe this is just a key value pair for a DBObject
• DBRelationship something to do with linking, assumed this simulates a PK:FK relation much like an relational database where a relationship exists like (AccountInfo -> CharacterInfo 1:M)
• DBBindingList specifically exists to support importing / exporting database (assumed via JSON).

Thoughts:

• Introduce BinaryDataSource along with BinaryJsonFileWriter / BinaryJsonFileReader to encapsulate File IO (currently owned by Session). The data source should only be responsible for consuming (saving) and returning (loading) List<DBObject>.
  • I have yet to consider whether an BinaryDBObjectSaving/LoadingAdapter may be required for saving specific types.
• Introduce DataFactory<T> to encapsulate creation of DBObject, including use of a IndexSupplier/Generator to replace logic currently owned by DBCollection.
• Introduce CachingBinaryDataRepository<T> to manage in memory collection, and delegating committing to file via BinaryDataSource.
• Introduce a ScheduledSavingDataRepository to manage the automated saving (currently in SEnvir).
• Introduce separate PrivateUserDataRepository, PrivateSystemDataRepository and PublicSystemDataRepository to create better separate between client data, and server data.

Considerations:

• Push all repository & collections into a GlobalTypeAwareDataRepository for ease of management backed by a type aware Diction<Type, DBObject> with Find<T>() and Save<T>(ob) methods instead of having the currently 20+ unmanageable InfoList functions.

To Be Considered:

• Where does AutoBackup logic go? It may be prudent to introduce two variants of BinaryDataSource. What manages the autobackup process? Further analysis required.