added a few tests, fixed bugs that those tests turned up, and refactored a thew things on the way #25
Conversation
| { | ||
| sectionLength = ASN1.decode_unsigned(buffer, offset, lenValue, out uint rawValue); | ||
| if(sectionLength == -1) | ||
| throw new InvalidOperationException($"{nameof(ASN1.decode_unsigned)} returned -1"); |
There was a problem hiding this comment.
The other DecodeEnumerate<> method doesn't throw exception in case of -1. Shouldn't this be consistent?
| 0x09, 0x12, 0x1C, 0x02, 0x00, 0x00, 0x04, 0x2C, 0x00, 0x00, 0x00, 0x0A, | ||
| 0x39, 0x00, 0x4E, 0x09, 0x55, 0x2E, 0x44, 0x42, 0x82, 0x00, 0x00, 0x2F, | ||
| 0x09, 0x6F, 0x2E, 0x82, 0x04, 0x00, 0x2F, 0x4F | ||
| }; |
There was a problem hiding this comment.
How about we add the expected bytes to the result of ASHRAE.F_1_2() and similar in other cases? This way we have one place where we store data taken from the specification annex.
| * I'm talking with, which inherits from a standard bacnet-decoder and can be extended / overridden. | ||
| * | ||
| * Also: cyclic dependencies. Bad thing. | ||
| */ |
There was a problem hiding this comment.
Ok, first of all I don't think we need a manifesto like this in the codebase. I think it's better to open up an issue and discuss it there. Just marking something for refactor or redesign like you did in other places would be better.
But to anwser it, of course I know what SoC is and I agree with your arguments (I don't know if this was directed to me or not but I anwser just in case). The problem is the legacy code I dind't write and that I didn't change much except of introducing those interfaces I think. I don't remember what I needed them for but for sure they made it easier dealing with this implementation (they only expose methodes that were already existing).
There was a problem hiding this comment.
I'm sorry if that sounded like criticism of the work that was done there - that wasn't my intention.
I came across this code during debugging, and it was completely unexpected to find it implemented this way, because it was implemented differently everywhere else.
Since I didn't know if this was very old code that hasn't been refactored, a contribution from someone who just did things differently, or if it is intended to be the new way of doing things, I just wanted to mark it up for discussion.
The reasoning is there because I tend to forget why I marked something as TODO, and/or things look less bleak on a different day, so it's basically a note to myself 😉
As for opening an issue: while splitting up the code and fixing issues, my impression was that there is much to do: inconsistent naming, duplicate code, strange ways of doing things. Some of those things changed back and forth a few times (sometimes I discovered why something was done this specific way), but I still think we may be better off re-imagining the whole encode/decode-stuff. I'm currently working on an experimental implementation to sharpen my idea of how this could be done differently.
There was a problem hiding this comment.
Sure, no harm done ;) At the beginning I hat little knowledge about BACnet protocol, but needed to communicate with BACnet device in order to integrate it. I liked the application YABE because it worked and was written in C# so decided to use its guts to do the job. I was trying not to break stuff at first but then the deeper I got into the code, the more frustrated I became, just like you I guess. So that you have a little perspective, please checkout and analyze the original code:
svn checkout https://svn.code.sf.net/p/yetanotherbacnetexplorer/code/trunk/Yabe
Everything is stored mainly in BACnetBas.cs (8523 LoC) and BACnetClient.cs (2724 LoC). And I checked, it contains ASN1.IASN1encode interface but no ASN1.IASN1decode so I guess I added it and refactored the names. Anyway, I think you will like our codebase a little bit more after you analyze the original and think that I'm open to changing it ;)
No description provided.