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

Refactor code based on review #3

Merged
merged 22 commits into from Jan 10, 2020
Merged

Refactor code based on review #3

merged 22 commits into from Jan 10, 2020

Conversation

@zsmb13
Copy link
Contributor

zsmb13 commented Jan 7, 2020

I've reviewed this code for the following article: https://zsmb.co/lets-review-pokedex/

These are the changes I'm suggesting in that article, take any or all of them as you wish! :)

zsmb13 added 22 commits Jan 7, 2020
@GuilhE

This comment has been minimized.

Copy link

GuilhE commented Jan 7, 2020

I was reading your blog post and I wanted to place a question. Since there's no way to comment (I don't use Twitter), and no "issues" tab is available in this repo, I'm going to comment on this pull request. Hope you don't mind ^^.

Instead of:

override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    pokedexViewModel = ViewModelProviders.of(this).get(PokedexViewModel::class.java)
}

Couldn't we just (I use Dagger and Data Binding):

    @Inject
    lateinit var viewModelFactory: ViewModelFactory
    protected val viewModel: ViewModel by lazy { ViewModelProvider(this, viewModelFactory).get(getViewModelClass()) }
    protected lateinit var binding: ViewBinding

    override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? {
        binding = DataBindingUtil.inflate(inflater, getLayoutResId(), container, false)
        binding.setVariable(BR.viewModel, viewModel)
        binding.lifecycleOwner = viewLifecycleOwner
        ...
        return binding.root
    }

What do you think of this?
Thanks for your time!

@zsmb13

This comment has been minimized.

Copy link
Contributor Author

zsmb13 commented Jan 7, 2020

First of all, I've now added an Issues tab over there for discussions, didn't realize that GitHub doesn't have one for forks by default (which I believe is what happened?).

And yes, sure, lazy initialization of a ViewModel makes sense, and should work perfectly fine! If that's how you use it, and it works for you, there's not much of a reason to change it.

However, since I know that I'll be using it at some point anyway (thus never actually avoiding its creation), I would probably just initialize it at one known point in the code, which makes it easier to reason about. Plus lazy has a tiny bit of overhead by default for its locking behaviors and checks for whether the value is initialized, which you could also avoid by just creating it in a lifecycle method.

(And then in my actual applications, I have ViewModel creation abstracted away by some base classes. so I never actually write code like this myself.)

@GuilhE

This comment has been minimized.

Copy link

GuilhE commented Jan 7, 2020

Yup all this kind of logic is abstracted in BaseClasses, normally my ChildActivities/Fragments, only have to "initViews" and manage the observe logic, etc.

Thanks for your input :)

@mrcsxsiq mrcsxsiq merged commit e579cac into mrcsxsiq:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.