Suggestion for improvement

[danielnaumau]

Hey, thanks a lot for taking your time and solving our task.

But we found some issues in your code which we would like you to fix:

1. Invalid .gitignore. You can take it from here https://alvinalexander.com/source-code/scala/sample-gitignore-file-scala-sbt-intellij-eclipse/ ;
2. Invalid usage of implicits. Please read more attentively what they do. In your case only the Decoder[_]s need to have the implicit modifier;
3. Try not to use mutable variables in isPointInPolygon;
4. Please move all json codecs in a separate file. You have a parser module, so it will be nice if only this module contains json related code;
5. Try to use for-comprehension in generateResults. And not only there, we use for-comprehensions to work with Eithers and Options;
6. Don't use exits and throws in the code;
7. Be careful when you work with lists. You have a lot of unsafe places where you call smth like pointsArray(0). Try to use get(index) instead. If the element doesn't exist, this function will return None instead of throwing an exception;
8. Please add unit tests!! It's not a mandatory rule but your code doesn't cover all edge cases, so with unit tests it will be easier to find them. Try to test your solution with empty polygons or smth like that;
9. ParserLocations and ParserRegions can be objects, then you can just ParserLocations.parseToType without creating an instance;
10. You can initialize your ...FilePath values without using mutable variables. Hint: you can collect the args into a Map[String, String] by doing args.sliding(2,2).collect { case Array(key, value) => (key, value) }.toMap.

[MartynoSiela]

@danielnaumau I've tried to address all the points in your suggestions.

6. Why is that? In some cases (my comment about #7) I see that I should use exit. But I guess that it is only because my code was structured not in a proper way from the very beginning. Is it a bad practice to use exits and throws in general or is it just "not the Scala way" of writing code?

7. Not sure if I did this correctly. get(index) does not seem to exist. I've refactored code for the custom decoders using lift(index) and matched to only continue when there are valid values but I'm not sure how to handle the invalid cases. I've added some inline comments about this issue.

8. I've added some unit tests. I've added an additional check in the isPointInPolygon method for the program to not crash when the polygon is empty. However i have some tests failing on edges and corners and also in the case of non conventional region (bowtie) at the intersection of two edges. I'm not sure how to address these issues. On the one hand it might be the issue with the ray casting algorithm or at least my implementation of it. On the other hand if the application is dealing with real world geographical regions and locations, in my opinion these edge cases would not be an issue because it would be highly unlikely for a location to appear exactly on an edge or a corner of a polygon.

9. I would appreciate a comment about how i restructured the code in main, it seems a bit weird this way.

Thanks again for your original feedback!

[danielnaumau]

Hey, sorry, I cannot give you detailed answers about that but I can write some hints

6. It's a very bad practice. For comprehension can solve your problems
7. Lift(index) is fine, for Invalid cases you can use Either or Option.https://blog.rockthejvm.com/idiomatic-error-handling-in-scala/
8. Error Handling ^^
9. For comprehension is your answer