serverstf / python-valve

A Python Interface to various Valve products and services
Other
236 stars 71 forks source link

Switch message decoding from declarative to imperative syntax #57

Closed Yepoleb closed 4 years ago

Yepoleb commented 6 years ago

I tried implementing the extra data fields in the A2S_INFO response today and wasn't satisfied with how it went. I spent a lot of time scrolling around, trying to figure out how the code works and how to implement the new conditional fields. After like 2 hours I came up with this approach, which is not well implemented and quite ugly in my opinion: https://github.com/Yepoleb/python-valve/commit/e8a65c43371db462b29015bd1419cadc7a412368.

Because I wasn't at all satisfied with this, I tried a different approach. I've had a small custom ByteReader class laying around from previous projects and used that to re-implement the InfoResponse decoding. It took around 10 minutes of almost zero effort programming, the extra fields making up about 2 minutes. There's some basic error handling missing, but the decoding part looks nice and clean to me. Here's that code: https://github.com/Yepoleb/python-valve/commit/ee8b97cd6cb122f7794817b900483aa3e9ae6cac

I think the main benefit of declarative programming, which is saving you from writing the same basic function calls over and over, doesn't come into play here. Message classes are only either encoded or decoded, so all we're left with is the massive overhead of the abstractions. Imperative syntax on the other hand is very flexible and straight forward, because all the logic is contained in a single method.

What do you think? I'm willing to do all the rewriting if this suggestion gets accepted, so only worry about future changes.

Yepoleb commented 6 years ago

Implementation is now finished in the byteio branch.