Chore: Streamline the architecture of Connective Rooms #647

mikhail-dcl · 2024-04-18T07:40:33Z

The goal of this PR

Simplify and streamline the architecture behind ConnectiveRoom and related structures

Problems

№1. Cyclic dependencies on the same interface in `Decorators`

Inability to infer the call graph looking at the code solely
Excessive range of options as everything is under the same interface.
Deep nesting which is possible to start unwinding from the Container only

Before:

№2. Over-usage of abstraction, forceful contracts merrying

Inability to understand the applicability of a certain final type when everything is abstracted and within the same contract hierarchy. Tendency to violate YAGNI considerably
Functionality of the core is hidden behind the same interfaces as many other Decorators use. Difficulty to say where is the core and where is not. Pretending the core can be replaced with a higher level entity.

Before:

After:

Before:

After:

№3. Hiding different layers of abstraction behind the same contract

The top most-layer:

Before:

After:

Clarification of the core usage

№4. Proxies over-usage

ℹ️ InteriorRoom is a proxy to allow re-assigning the object without consumers' awareness of the object re-assigned

Before:

InteriorRoom already provides re-assigning functionality but RenewableDecorator is built to provide this functionality again

After:

The Core is fully under control of the higher levels.
InteriorRoom is created once on the top-most layer and propagated as an argument
No need for the extra Decorator: it has gone completely

Summary

RenewableArchipelagoIslandRoom Decorator has gone as it was fully redundant as duplicated the functionality of InteriorRoom
Archipelago and Gatekeep were moved to their own layer that stands above the core: with their own contracts as they do not match 100% with the core
The Core is under its own exclusive contract but the interface is not really used because the core was not used in an abstract manner, and no test coverage was provided either.
After that there we no decorators left that could operate on the same layer as the core extending its functionality.
As I suspected there was no need for such level of abstraction and when layers were broken down and put under their own minimally needed contracts, the consumer side didn't require any changes.

Extra

I introduced a separate multiplayer container to juggle with dependendencies there

* Remove the over-usage of Decorator * Separate interfaces according to the layers of abstraction * Get rid of the bottom-less proxying of InteriorRoom * Adjust un-adapted asynchronous calls

NickKhalow

I left the comments

UPD: also for some reason the RealmRoom won't start for me at all, did you test your changes?
https://drive.google.com/file/d/1RZpTEF37u6rJTVSWMpG0f8cfz3-oxMyS/view?usp=sharing

Explorer/Assets/DCL/Multiplayer/Connections/Archipelago/Rooms/IRealmRoomStrategy.cs

NickKhalow · 2024-04-18T11:34:22Z

Explorer/Assets/DCL/Multiplayer/Connections/Archipelago/Rooms/IRoomProvider.cs

+    /// This interface is not connected to <see cref="IConnectiveRoom"/>.
+    /// It has some common fields but its contract can mutate independently
+    /// </summary>
+    public interface IRoomProvider


What's the point of IRoomProvider? It doesn't jst "Provide" or it's not being a just source of IRoom, it acts as an object that handles the lifecycle of the room

It has one single purpose: Constructs either a fixed connective room or a dynamic one based on the archipelago.
Regarding the name can you propose a better one? It's not necessary to go by book, anything that would feat its purpose.

I think IConnectiveRoom suits great, if you are afraid of exposing other implementations we can rename internal one ICoreConnectiveRoom

The thing is it's not the Room itself, it's an upper layer that provides one of the rooms according to the inner conditions.
Well, in fact it can be a connective room in a similar meaning as the factory can be abstract. But why the core then should be connective?

NickKhalow · 2024-04-18T11:38:26Z

Explorer/Assets/DCL/Multiplayer/Connections/Archipelago/Rooms/RealmRoomsProvider.cs

+        private FixedConnectionRoomStrategy CreateFixedConnectionRoomStrategy(string adapterUrl) =>
+            new (sharedRoom, webRequestController, adapterUrl);
+
+        private ArchipelagoIslandRoomStrategy CreateArchipelagoIslandRoomStrategy(string adapterUrl) =>
+            new (sharedRoom, identityCache, archipelagoSignFlow, characterObject, adapterUrl);


Sharing an InteriorRoom can bring possibilities of mutating from different places that can be hard to debug

Why don't isolate it per each instanse?

According to our application it's not needed to be isolated in every instance

Having it per instance created the problem with multi-leveled proxies when inside proxy there is another proxy, and so on and so forth: I described it in the PR

that can be hard to debug

Not sure if it is hard to debug: the source of InteriorRoom is streamlined and on purpose I no longer expose InteriorRoom from the core

NickKhalow · 2024-04-18T11:40:33Z

Explorer/Assets/DCL/Multiplayer/Connections/Archipelago/Rooms/RealmRoomsProvider.cs

+        private FixedConnectionRoomStrategy CreateFixedConnectionRoomStrategy(string adapterUrl) =>
+            new (sharedRoom, webRequestController, adapterUrl);
+
+        private ArchipelagoIslandRoomStrategy CreateArchipelagoIslandRoomStrategy(string adapterUrl) =>


I remember points from you about that this object is factory, why it has hardcoded dependencies and acts as Factory, but named as strategy?

Yup, Factory should not contain a reference to the produced object. But I agree. Possibly it should be a factory, and then it should capture a URL argument to produce the object. I will change it to see if it is better

I tried to create a factory by book from it but it would require capturing variables (IConnectiveRoom) and I don't see any sense of doing it when we can keep them in the class.

I don't want to introduce any other layers just because it does not fit the name of Factory or Strategy. If you know a commonly accepted name for a type that constructs an entity and holds a reference to it, I am all ears.

We can create a IRealmRoomFactory, and inject it inside the object

I don't think merging 2 concerns Factory/Strategy into a single object is a good Idea

I aim for simplicity: I don't see the reason for 2 layers here. It's simply not needed

Explorer/Assets/DCL/Multiplayer/Connections/RoomHubs/RoomHub.cs

Explorer/Assets/DCL/Multiplayer/Connections/Rooms/Connective/IConnectiveRoom.cs

NickKhalow · 2024-04-18T11:45:03Z

Explorer/Assets/DCL/Multiplayer/Connections/Rooms/Connective/RenewableConnectiveRoom.cs

@@ -1,4 +1,4 @@
-using Cysharp.Threading.Tasks;
+/*using Cysharp.Threading.Tasks;


I see you just commented it, what is it replaced on?

Not needed: I described it in the PR: its logic is already contained in InteriorRoom: no sense of making a proxy over proxy again

NickKhalow · 2024-04-18T11:52:26Z

...er/Assets/DCL/Multiplayer/Connections/Archipelago/Rooms/Fixed/FixedConnectionRoomStrategy.cs

+    /// <summary>
+    /// No need for any abstractions - it's a unique behaviour that we can't replace
+    /// </summary>
+    internal class FixedConnectionRoomStrategy : IRealmRoomStrategy


Define please the "abstractions", something you are naming it and claiming it's bad to have many of it and something not

Super simple interface:

One level - no nesting

One member IConnectiveRoom ConnectiveRoom { get; }, two implementations

Clear purpose: to allow select polymorphic behavior between Fixed and Archipelago realms

Streamline the architecture of Connective Rooms

92ad709

* Remove the over-usage of Decorator * Separate interfaces according to the layers of abstraction * Get rid of the bottom-less proxying of InteriorRoom * Adjust un-adapted asynchronous calls

mikhail-dcl marked this pull request as ready for review April 18, 2024 08:42

mikhail-dcl changed the title ~~Streamline the architecture of Connective Rooms~~ Chore: Streamline the architecture of Connective Rooms Apr 18, 2024

NickKhalow requested changes Apr 18, 2024

View reviewed changes

mikhail-dcl added 3 commits April 18, 2024 15:20

Remove RenewableConnectiveRoom

5ace9a1

Use IConnectiveRoom to enable mocking

0ab9a6a

Start Rooms in parallel

b43ca5a

mikhail-dcl added the do not merge label Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Streamline the architecture of Connective Rooms #647

Chore: Streamline the architecture of Connective Rooms #647

mikhail-dcl commented Apr 18, 2024 •

edited

Loading

NickKhalow left a comment •

edited

Loading

NickKhalow Apr 18, 2024

mikhail-dcl Apr 18, 2024

NickKhalow Apr 19, 2024

mikhail-dcl Apr 23, 2024 •

edited

Loading

NickKhalow Apr 18, 2024

mikhail-dcl Apr 18, 2024

mikhail-dcl Apr 18, 2024

NickKhalow Apr 18, 2024

mikhail-dcl Apr 18, 2024

mikhail-dcl Apr 18, 2024

NickKhalow Apr 19, 2024

mikhail-dcl Apr 23, 2024 •

edited

Loading

NickKhalow Apr 18, 2024

mikhail-dcl Apr 18, 2024

NickKhalow Apr 18, 2024

mikhail-dcl Apr 18, 2024

		@@ -1,4 +1,4 @@
		using Cysharp.Threading.Tasks;
		/*using Cysharp.Threading.Tasks;

Chore: Streamline the architecture of Connective Rooms #647

Are you sure you want to change the base?

Chore: Streamline the architecture of Connective Rooms #647

Conversation

mikhail-dcl commented Apr 18, 2024 • edited Loading

The goal of this PR

Problems

№1. Cyclic dependencies on the same interface in Decorators

№2. Over-usage of abstraction, forceful contracts merrying

№3. Hiding different layers of abstraction behind the same contract

№4. Proxies over-usage

Summary

Extra

NickKhalow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhail-dcl Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhail-dcl Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhail-dcl commented Apr 18, 2024 •

edited

Loading

№1. Cyclic dependencies on the same interface in `Decorators`

NickKhalow left a comment •

edited

Loading

mikhail-dcl Apr 23, 2024 •

edited

Loading

mikhail-dcl Apr 23, 2024 •

edited

Loading