snowplow-incubator / sauna

:hotsprings: A decisioning and response platform
https://github.com/snowplow/sauna/wiki
70 stars 11 forks source link

WIP - Urban Airship Responder #35

Closed rmanojcit closed 8 years ago

rmanojcit commented 8 years ago

Urban Airship Responder added - version 1

alexanderdean commented 8 years ago

Thanks @rmanojcit - assigning this to @chuwy to do a deep review...

chuwy commented 8 years ago

Hello @rmanojcit

Just before dive deeply into review I want to give you few general advices on how to make your Scala (but most of these advices also applicable for other high-level programming languages) codebase look more elegant and easier to maintain.

At very first glance, formatting may seem very minor thing as it doesn't directly affect work of your program. But when time comes and someone need to read and maintain code - you can do him a favor by sticking with strict formatting guide. It's much easier to read code when it's predictable and well-organized. Things like unused variables and imports, redundant semicolons (we can omit them in Scala), stale comments, inconsistent indentations, etc will result in same byte-code, but will make your codebase harder to read as they're visual garbage and distract developer from what really matters - how your code works.

There's no absolute rules, but I can advice you to have a look someday (not necessary now) to following guide: Databricks Scala Guide. Also Python's PEP8 is good source of inspiration. Long story short: try to avoid unnecessary symbols and elements, as they're visual garbage, insert additional line-breaks when they can separate independent sets of statements which help developer to concentrate on particular part of logic, preserve particular amount of spaces between symbols (val a = 3 vs val a=3).

Formatting can be seen as part of more broad programming style. As I said styling important probably for all languages, but for Scala especially because:

Now, let me point few moments that are more about coding style and after they will be fixed we could concentrate on more important part - IO and async.

chuwy commented 8 years ago

Hello again Manoj!

There's also a little problem in your MaptoRequest method, since it uses Futures yet looks blocking (because of Thread.sleep) which is bad idea in akka actor. So, let's now concentrate on little refactorings of huge Unit-typed methods into small composable "pure" functions and after that we can actually work on the Future/Akka side of your code.

rmanojcit commented 8 years ago

ya sure! thanks Anton I will look into all the stuff you have mentioned.

chuwy commented 8 years ago

Thanks @rmanojcit. Feel free to ask if you have any questions. Just remember: functions are cheap, bugs are not! It's always better to have lot of small composable parts. Good luck.

chuwy commented 8 years ago

Hey @rmanojcit! I see impressive advancement! Please ping me if you think this is ready for next review.

chuwy commented 8 years ago

Hey @rmanojcit!

Just a quick update. If you're still struggling with immutable collections and vals you can find these couple of articles useful:

But of course it is probably better to just ask me if you have questions.