[Enhancement] Maintainability Ideas
Opened this issue · 8 comments
- Enums for MessageType, Add Enum Extension Methods
We can use Extension Methods to get a [Description] tag off of an Enum. This adds a little overhead, since we have to get the string at runtime as opposed to just reading the string.
Example:
using System.ComponentModel;
namespace VTS
{
public static class EnumExtensions
{
public static string GetDescription<T>(this T value)
{
var fi = value.GetType().GetField(value.ToString());
var attributes = (DescriptionAttribute[])fi.GetCustomAttributes(typeof(DescriptionAttribute), false);
return attributes.Length > 0 ? attributes[0].Description : null;
}
}
}
Pros:
Less typos, easier to develop/maintain.
Cons:
Less performant. Not sure if that is an issue, since it is all (usually) running on a local network.
- Remove Serializable, adding Newtonsoft
Newtonsoft makes it so we don't need to mark every class inVTSDataas[Serializable]. This is marking a class being Binary Serializable in C# and is generally frowned upon for various reasons, especially since we just need to serialize it for JSON data.
Pros:
Not using [Serializable].
Cons:
Adding a library (Newtonsoft) to the project.
- Rework Data, remove Data from subclasses, merge onto VTSMessageData
I was thinking we could just use Inheritance and Generics here to not create a newDataon every class. Here is an example of howVTSMessageDataandErrorDatacould look
public class DTO {}
public class VTSMessageData<T> where T : DTO
{
public string APIName = "VTubeStudioPublicAPI";
public long Timestamp;
public string APIVersion = "1.0";
public string RequestID = Guid.NewGuid().ToString();
public string MessageType;
public T Data;
public VTSMessageData(MessageTypeEnum e)
{
MessageType = e.GetDescription();
}
public VTSMessageData(MessageTypeEnum e, T d)
{
MessageType = e.GetDescription();
Data = d;
}
}
public class VTSErrorData : VTSMessageData<ErrorData>
{
public VTSErrorData() : base(MessageTypeEnum.APIError) { }
public VTSErrorData(string e) : base(MessageTypeEnum.APIError, new ErrorData(e)) { }
public class ErrorData : DTO
{
public ErrorData() { }
public ErrorData(string message) { Message = message; }
public ErrorID ErrorID;
public string Message;
}
}
Pros:
Code is a bit easier to work with and expand upon.
Cons:
Maybe a little less understandable at a glance?
Currently need to use Static Usings, IE: using static VTS.VTSArtMeshListRequestData; to access the 'Data' types, which will clutter usings in some files.
I'll try to think of a better way to handle this though.
- Remove 'This', general code/linting cleanup
There are lots of small linting things that bug me a bit, like unnecessarythis., variable name casing, null coalescing, null propagation, and other VERY MINOR things. This isn't so much a problem, but thought I'd bring it up and see what your thoughts on it were. I could make these changes in a pull request, but wanted to see if this was overstepping my bounds a little.
Example:
This:
if(this._ws != null){
this._ws.Update(timeDelta);
}
Becomes:
_ws?.Update(timeDelta);
Pros:
My Brain will process this better?
Cons:
Your/Other People's Brains may not?
- Use Reflection in ProcessResponses
We can make use of Reflection in C# to remove the switch statements inProcessResponses, allowing for not having to manually update this whenever we add a new type. This example also uses(dynamic), which I would like to refactor out, since this also adds some extra overhead along with the use of Reflection. I just haven't thought of a better way of doing this yet.
Example:
private void ProcessResponses()
{
while (_ws?.GetNextResponse() is string data)
{
var response = JsonConvert.DeserializeObject<VTSMessageData<DTO>>(data);
Type responseType = Type.GetType("VTS" + response.MessageType + "Data");
var responseData = JsonConvert.DeserializeObject(data, responseType);
try
{
if (_events.ContainsKey(response.MessageType))
{
_events[response.MessageType].OnEvent((dynamic)responseData);
}
else if (_callbacks.ContainsKey(response.RequestID))
{
if (response.MessageType == "APIError")
{
_callbacks[response.RequestID].OnError((dynamic)responseData);
}
else
{
_callbacks[response.RequestID].OnSuccess((dynamic)responseData);
}
_callbacks.Remove(response.RequestID);
}
}
catch (Exception e)
{
VTSErrorData error = new VTSErrorData(e.Message)
{
RequestID = response.RequestID
};
_events[response.MessageType].OnError(error);
}
}
}
Pros:
Easier to maintain.
Cons:
Uses more computation.
Please let me know if any of these possible enhancements interest you! I'll make some pull requests with these features separately. I already have some of this work done in one, gross, big, commit I was working on as a test over at Millyarde@e3851fb. I'll probably wait until version 2 is in main to make these changes, if that isn't too far off!
- I do not understand what enum we are looking to add a "description" to. The ErrorType enum is provided by Denchi, I just copy the latest in every update.
- Newtonsoft appears to use the MIT license for redistribution so it's technically feasible, though it seems like a lot of bloat for something that's already being accomplished natively. I would also worry that this introduces the opportunity for version conflicts, if users are already using Newtonsoft in their project.
- I do not understand how this is functionally different from what I'm currently doing. There is no overlap or common field among the child
Dataclasses, so there doesn't appear to be any benefit in having them inherit from a common ancestor. - You've got me here: my use of
thisis a holdover from my day job as a JavaScript monkey. Because of the IDE color palette I use, prepending things withthismakes them pop on a way that's easy for me to read. Additionally, Unity does not support the null-coalesce operator well on things that can be serialized in the Inspector, for reasons that have been lost to the sands of time. I also generally think that the null-coalesce operator is a lot easier to miss than an if-block when skimming over code. - I do not have high confidence that reflection will not cause a performance hit while dispatching requests at 60FPS, based on my experience with reflection in other languages (Java). I also have no idea what
dynamicdoes. 😅
Hope that helps address things!
- I was thinking of adding an enum for MessageType with a
[Description]tag of the API, so that we have a 'strongly typed' reference to the message types, as opposed to the current scattered 'magic strings' of the message type, in case they change one or something in the future. Does add some degree of overhead since you have to use Reflection to grab the string off the enum instead of just... using a string, but it's pretty minor overhead for the safety and usability it provides imo. - Yeah, definitely valid points. I reflexively try to remove any serializable tags due to past traumas caused by binary serialization in C#. It seems like you need that in Unity for seeing those fields in the Inspector, but you won't need it for any non-unity touching code, as that
[Serializable]tag is only for Binary Serialization and has nothing to do with JSON Serialization. Totally understand wanting to avoid version conflicts as well, Streamer.bot (which I'm trying to use your library to make a clean extension for it), has it's own version of WebsocketServer and Newtonsoft, dealing with versioning with C# is headaches for days 😭 - It isn't functionally different, just a little more maintainable pattern imo. Looks cleaner to me anyway, but I'm not sure how often you have to add new classes like this due to API updates, so may not even be worth doing 🤷♀️
- It's not broke and it works, so it's fine🤣. That's why they call it linting, since it isn't important. Just figured I'd mention some of the general stuff I saw. Totally didn't know about that Unity thing, that's really dumb since that's a basic C# 6 thing, and we are on like C# 11 now 🤯. Kinda glad I decided to start working on a different project in Unreal instead of Unity now 😅
- Yeah, I was concerned about this as well, since people run VTS on a variety of hardware and this is part of lifecycle events. Was curious on your take on the idea. The Dynamic keyword in C# basically is an expensive cheat on skipping compile-time type checking. It's like using
anyin Typescript, except with performance drawbacks greater than reflection even 🤮, definitely a no-no here, but it has it's uses in some places.
So it seems like "No"'s on everything?
- This makes sense to do. I don't like the magic string approach, as it's prone to typos (as you have personally seen and corrected)
- I still don't understand what's wrong with the
Serializableattribute. - I'll think about it.
- This totally comes down to personal preference. I should install an actual linter for home use. Right now I just have a formatter.
- I think we can compromise somewhere in the middle, making an enum list of message types but still having a precompiled switch statement for each case, to eliminate the need for reflection.
We could just do const strings instead of an enum as well for the message types, that's a little less overhead than the enum description tag approach. Would lose a little bit of code flexibility, but it is a little more performant.
Yeah, the other thing is that outgoing messages all need to be SomethingRequest and the incoming messages are all SomethingResponse. Unless we wanted two unlinked enum values each, we could probably get away with making the enum value just Something and having the Send method append the word Request to it. Similarly, the receiving switch could attempt to trim Response from the string value to find the matching enum.
I wish C# allowed for complex enum objects like Java does. That's the one word of praise I'll give the language.
EDIT: I'm just now comprehending that we can just make two custom attributes:
enum MessageTypes {
[Request("SomethingRequest")]
[Response("SomethingResponse")]
SOMETHING,
}
This is functionally done. I'm kicking the enum-ification down the road a bit, but you can use the code from the branch: https://github.com/FomTarro/VTS-Sharp/tree/unity-decouple safely now. I do not anticipate further changes. I am presently writing the migration steps in the README, which you presumably do not need for your purposes.
Sorry for the month of delay.
Sounds good! I'll take a look at the changes and hopefully work on some stuff with this next week.
Saw the to/from JSON abstraction, Newtonsoft addition, and removing the meta files though, looks great and will make things easier for my little fork!
No worries on taking a while, figured this would be a longer work in progress and some stuff I could help add/code review if it was stuff you wanted 😅
I got busy myself so I hadn't really had a chance to work on stuff anyway. We can close this out if you don't like seeing the "1 issue" and I can just send in PR's for stuff like the enums if/when I get to it. Whatever works best for you, thanks for the hard work 😊
The 2.0.0 release is officially out. 2.1.0 will begin a serious investigation of enumerating message types and deprecating the use of strings as much as possible.