Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert firestore quickstart to Kotlin #638

Merged
merged 8 commits into from
Sep 7, 2018
Merged

Conversation

samtstern
Copy link
Contributor

Fixes #630

Change-Id: I3995228bb5b924cf3d63a1906dd8b461088759c0
Change-Id: Ieeeebb59750672b9126f9d039a8736341a640492
Change-Id: I1e7a8d63b8b65fbbcefaaddc11c8b8008bfc55af
Change-Id: I0f71b80b14306b9a790b3663b23e3bdb4c63f8ac
@samtstern samtstern changed the title [WIP] Convert firestore quickstart to Kotlin Convert firestore quickstart to Kotlin Sep 7, 2018
@samtstern
Copy link
Contributor Author

@the-dagger @rosariopfernandes if either one of you has time to review, this one is ready to go.

If not I can get someone on my team to review, don't need to take up all of your time!

Change-Id: Id4fcd606fbe8b529fb6c2c2c9acd4b1f42c92111
@thatfiredev
Copy link
Member

@samtstern Sure, I'll have a look.

}

fun onSearchClicked() {
if (filterListener != null) {
Copy link
Contributor

@the-dagger the-dagger Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this can be replaced with a safe call. ?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would do:

filterListener?.let{
    filterListener.onFilter(filters)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, correct.
Also, I think that

filterListener?.onFilter(filters)

would work just fine as well, no need for the let block.

What do you think @rosariopfernandes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PERFECTION


// Sort by (orderBy with direction)
if (filters.hasSortBy()) {
query = query.orderBy(filters.sortBy!!, filters.sortDirection!!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of !! can we use toString() on these params?


override fun onResume() {
super.onResume()
dialog.window!!.setLayout(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think that we should avoid the not-null assertion operator (!!) as much as we can.
Leads to undesirable NPEs!

https://kotlinlang.org/docs/reference/null-safety.html

restaurantFormRating.rating.toDouble(),
restaurantFormText.text.toString())

if (ratingListener != null) {
Copy link
Contributor

@the-dagger the-dagger Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, can be replaced with a safe call operator ?.

/**
* Model POJO for a rating.
*/
class Rating {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a data class here?

* Restaurant POJO.
*/
@IgnoreExtraProperties
class Restaurant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data class here as well

Copy link
Contributor

@the-dagger the-dagger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samtstern added some pointers.
Most of it looks good!

Change-Id: I063d6190ca8136a83249b37c48e2d643df74067e
Change-Id: I08a033f6a890efca393da5faa073ba1495273af5
Change-Id: I1c97a006b23e32f5b35a7e8640b83c42b98a323a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants