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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
