[GitHub] [avro] KhrystynaPopadyuk commented on pull request #1833: AVRO-3001 AVRO-3274 AVRO-3568 AVRO-3613: Add JSON encoder/decoder for C#

[GitBox <gi...@apache.org>]

KhrystynaPopadyuk commented on PR #1833:
URL: https://github.com/apache/avro/pull/1833#issuecomment-1229103595

   Hi @martin-g ,
   
   Please find my 5 cents below.
   
   As was mentioned in description this is port from Java and it looks 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 and I believe should be address:
   
   1. Folder and file structure. For example:
    - Why do not create separate folder for JsonParser and placed all related classes there
    - "ParsingDecoder" (https://github.com/apache/avro/blob/c13a45debf4594dc7bc3c90aef64cee5ae115a5d/lang/csharp/src/apache/main/IO/ParsingDecoder.cs) It stated that this is "Base class for a <see cref="Parsing.Parser"/>-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?
    - 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. 
    - 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 that should be use them carefully and with attention. I have founded 38 static items in Symbol class. That looks very suspicious. 
   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.
   4. switch operator - is there way to reduce usage of switch/case?
   
   
   


-- 
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: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org