rayokota commented on PR #1833: URL: https://github.com/apache/avro/pull/1833#issuecomment-1229131964
> Hi @martin-g and @rayokota , Hi @KhrystynaPopadyuk , > > Please find my 5 cents below. > > As was mentioned in description this is port from Java and it looks and feels exactly as port. Of course this code will work. But it would be very difficult maintain, extend and provide sufficient unit test coverage. This is new functionality and it is easy refactor it now, and would be very difficult update if later. > > Please find below few suggestion that, I believe, should be addressed: > > 1. Folder and file structure. For example: > > * Why do not create separate folder for JsonParser and placed all related classes there All the parsing classes are indeed in a separate `Parsing` package. The `JsonEncoder` and `JsonDecoder` are siblings of `BinaryEncoder` and `BinaryDecoder` in the `IO` package, which makes sense to me. > * "ParsingDecoder" (https://github.com/apache/avro/blob/c13a45debf4594dc7bc3c90aef64cee5ae115a5d/lang/csharp/src/apache/main/IO/ParsingDecoder.cs) It states that this is "Base class for a -based" but Parser (https://github.com/apache/avro/blob/c13a45debf4594dc7bc3c90aef64cee5ae115a5d/lang/csharp/src/apache/main/IO/Parsing/Parser.cs) does no have base class. Moreover ParsingDecoder is base class for JsonDecoder. So is "ParsingDecoder " correct name? What is the purpose of having this class? Why we need this additional layer of abstraction? The `ParsingEncoder` and `ParsingDecoder` are implementations of `Encoder` and `Decoder` that maintain some parsing state. The `JsonEncoder` and `JsonDecoder` subclass those classes to delegate the maintenance of the parsing state. In the Java code, there are also `ValidatingEncoder` and `ValidatingDecoder` that subclass `ParsingEncoder` and `ParsingDecoder`. I did not port those classes over. > * there are a lot of child classes and interfaces. For example file Symbol.cs contains not only abstract class Symbol but 17 child classes and one enum, And all definitions of fields, properties, child classes, enums, methods are without any structure and order. The `Symbol` class maintains the terminals and non-terminals of the parsing grammar, so that is why there are a lot of classes and enums in it. > * there are a lot of other things that need attention > > 2. Static method and classes is very controversial tool. It has own benefits but also it can brink a lot of negative. Overall we should be use them carefully and with attention. I have founded 38 static items in Symbol class. That looks very suspicious. I agree static methods and classes have their advantages and disadvantages, but I believe there is nothing wrong in the use of them in the `Symbol` class. I'd be happy to incorporate more specific suggestions at particular line numbers. > 3. Poor usage of interfaces but a lot of static and even hidden dependencies. That should be reviewed in direction to use interfaces as dependency instead of classes (even abstract). Dependency inversion/ dependency injection can bring a lot of benefit and flexibility to code. Dependency inversion also has its advantages and disadvantages, but I don't think it would really help here, unless you can be more specific. > 4. switch operator - is there way to reduce usage of switch/case? I think `switch` can also have its advantages and disadvantages, but I think its use is fine here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
