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

Cleanup #390

Merged
merged 9 commits into from
Mar 14, 2016
Merged

Cleanup #390

merged 9 commits into from
Mar 14, 2016

Conversation

F43nd1r
Copy link
Member

@F43nd1r F43nd1r commented Mar 13, 2016

Cleanup code.
These are changes which I would consider an auto-include, because they simplify, improve readybiliity or stability without introducing use-changes.
But there is still room for discussion.
I have started to split up my big pull request (I will remove it as soon as I'm done), so it can be discussed and approved/rejected part by part. This is the second part.

@william-ferguson-au
Copy link
Member

FIx those 2 and it's good to go. Nice work!

@william-ferguson-au william-ferguson-au merged commit 59a6af6 into ACRA:master Mar 14, 2016
@F43nd1r
Copy link
Member Author

F43nd1r commented Mar 14, 2016

@william-ferguson-au One more question: Is there a reason why values in ACRAConfiguration and ConfigurationBuilder are almost always boxed? Doesn't this lead to unnecessary possible Nullpointers?
If you agree I'll replace code like @NonNull Integer with int, which seems much cleaner IMO. Even if the values have to be saved boxed, I think setters still should take primitives.

@william-ferguson-au
Copy link
Member

I'd need to look at each in turn for a definitive answer.

But generally speaking if there is no reason for them to ever have a null value then yes making them primitives makes sense. But sometimes null is far more clear than an arbitrary value like 0.

@F43nd1r
Copy link
Member Author

F43nd1r commented Mar 14, 2016

Most of the getters in ACRAConfiguration already return primitives and all usecases I have already looked at assume that they are NonNull.
This results in the following:

  1. Values in ACRAConfiguration should be primitive
  2. Values in ConfigurationBuilder can be Null. If so, the builder returns the default. (this makes sense and should stay this way)
  3. Setters shouldn't accept null in either of these classes as this doesn't make sense from a user-perspective.

@F43nd1r F43nd1r mentioned this pull request Mar 14, 2016
@william-ferguson-au
Copy link
Member

Setters in the ConfgurationBuilder may need to accept null if that means that you are unsetting a value.

@F43nd1r
Copy link
Member Author

F43nd1r commented Mar 14, 2016

I can't think of any situation where it could be useful to set a value to default - without knowing what the default is. Because if you know what the default is, you can just pass it to the method. If you don't know what the default is you would create an from your perspective undefined environment. Also values are unset by default, so to set one to default you can just not touch it. But if you disagree I can revert that in the PR.

By the way - why is there no no-args-constructor in ConfigurationBuilder (in case someone wants to configure solely in code)?

@william-ferguson-au
Copy link
Member

I can't think of a normal use case that would require unsetting a value as we only ever expect to build a single ACRAConfiguration. But generally speaking a Builder is just a configurable factory and should be able to construct many items. If during the Builder's lifespan it needs to revert some current config to default then it needs the ability to unset that value.

This might be useful in ACRA's case if there is some complicated set up required, or if we needed to do it for testing. But I'm not that worried about it . Let it ride for now. We can always change it going forward. NB because of the defaulting behaviour in the Builder there was not much room for NPEs.

And yes, the no-arg constructor is required for those doing totally programmatic config. Personally it looks like pain to me, but there were howls when I attempted to get rid of it last time.

@F43nd1r
Copy link
Member Author

F43nd1r commented Mar 14, 2016

I think you misunderstood: There is no such constructor in ConfigurationBuilder. I was asking if there should be one.
Sidenote: There is a no-args constructor in ACRAConfiguration, but it will always result in a NullPointerException.

@william-ferguson-au
Copy link
Member

Sorry, I should have looked at the code first.

There is no need for a no-arg constructor in ConfigurationBuilder. Every app has an Application instance. It may or may not have ACRA config on that Application.

There is also no need for the no-arg constructor on ACRAConfiguration. Serialization doesn't need it and nothing can call it without getting an NPE. Remove it.

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.

2 participants