Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/847#issuecomment-112954221
Hey @shghatge,
this is a great first try, you got the logic right and I really like the
detailed javadocs ^^
I left a few inline comments, which should be easy to fix.
Let me also elaborate a bit on some general guidelines:
- Code formatting: we don't really have a strict Java code style, but there
a few things you can improve. For your code to be readable, it is nice to leave
a space after the commas separating arguments. For example `myMethod(arg1,
arg2, arg3)` instead of `myMethod(arg1,arg2,arg3)`.
We usually separate the closing of a parenthesis and the opening of a curly
bracket with a space, i.e. `myMethod() { ... }` instead of `myMethod(){ ... }`.
Also, try to avoid adding new lines if they are not needed.
Regarding the types missing, this is not creating an error, but gives a
warning. You can turn on warning notification settings in your IDE to avoid
this.
- I like it that you added separate methods `includeFields` methods` for
vertices and edges. It would probably make sense to do the same for the rest of
the methods. For example, you might want to skip the first line in the edges
file, but not in the vertices file. Right now, you are forced to either do both
or none. Alternatively, we could add parameters to the existing methods, to
define the behavior for edges and vertices files separately. For example
`public GraphCsvReader lineDelimiter(String VertexDelimiter, EdgeDelimiter)`.
What do you think?
- Finally, in order to catch issues like the one with the null
`VertexReader`, you should always try to test as much functionality you have
added as possible. In this case, it would be a good idea to add a test reading
from edges only and some tests for the different methods you have added.
Let me know if you have questions!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
