Skip to content

Code cleanup & annotations - #368

Merged
william-ferguson-au merged 7 commits into
ACRA:masterfrom
F43nd1r:master
Feb 16, 2016
Merged

Code cleanup & annotations#368
william-ferguson-au merged 7 commits into
ACRA:masterfrom
F43nd1r:master

Conversation

@F43nd1r

@F43nd1r F43nd1r commented Feb 11, 2016

Copy link
Copy Markdown
Member
  • updated gradle file
  • migrated to java 6/7 where supported by android (multicatch, diamond, switch with strings) (suppressed wrong lint suggestions)
  • removed code which supported lower android versions
  • replaced reflection with api level checks when appropriate (might increase performance, definetly helps debugging) (There was at least one reflection call with wrong arguments)
  • I've placed annotations last in case they are unwanted. I think it helps to know what arguments a method can / can't handle etc.
  • Afterwards I noticed some bad map iterations and boosted them

In some places I've replaced Stringbuilders with String concatenation. It is easier to read and the compiler transforms it anyways.

Note: some of these commits where split by hand and might contain data of later / newer commits.

@F43nd1r

F43nd1r commented Feb 11, 2016

Copy link
Copy Markdown
Member Author

I am only building with gradle, so the annotation dependency might need to be added elsewere too.

@F43nd1r

F43nd1r commented Feb 13, 2016

Copy link
Copy Markdown
Member Author

I did not realize before that using these features actually requires jdk 7. So: you probably don't want to merge these. I'll revert those changes which would prevent compiling with java 6 asap, so the useful stuff in here can be integrated.

@F43nd1r

F43nd1r commented Feb 13, 2016

Copy link
Copy Markdown
Member Author

If you don't like the annotations, tell me. Then I'll remove them from this request.

@william-ferguson-au
william-ferguson-au merged commit 7f92a80 into ACRA:master Feb 16, 2016
@william-ferguson-au

Copy link
Copy Markdown
Member

Thanks @F43nd1r.

I did a couple of little cleanups, but the only thing of note was that several of the ReportSenderFactorys were marked as returning @nullable when they should have been @nonnull.

Q: What happens at runtime if a client defies the annotation and passes in a null to a parameter marked @nonnull ?

@F43nd1r

F43nd1r commented Feb 16, 2016

Copy link
Copy Markdown
Member Author

These annotations are compile time only. @ nonnull prevents a compile if null is passed and should show a warning when a potentially null parameter is passed. At Runtime this will still result in a Nullpointerexception. Those are meant to help developers preventing Nullpointers beforehand and also allow to use methods correctly without reading sources. (I remember one place where I found a potential Nullpointer in acra and added a check, so at least for that it helps)

As I said, most of the annotations were generated automatically (because I haven't read the whole source code). If some of these are wrong, change them.

@F43nd1r

F43nd1r commented Feb 16, 2016

Copy link
Copy Markdown
Member Author

Here are two examples how these annotations work in Android Studio:
@ CallSuper
unbenannt
@stringres
unbenannt2

@william-ferguson-au

Copy link
Copy Markdown
Member

Just to clarify, if I annotate a param as @nonnull then a runtime check is made on that param. And a NPE will be thrown if it is null?

@F43nd1r

F43nd1r commented Feb 17, 2016

Copy link
Copy Markdown
Member Author

No, the NPE is thrown (as without the annotation) as soon as you try to access the parameter.
These annotations cannot enforce anything (you can even suppress them, but as long as you don't know what you're doing you should avoid that)
unbenannt
(Note that it says @ NotNull in the text, but the annotation is @ NonNull. I don't know why this is the case)

@william-ferguson-au

Copy link
Copy Markdown
Member

Hmm, that's problematic.
See https://github.com/ACRA/acra/blob/master/src/main/java/org/acra/ACRA.java#L188

I switched the tag for the config param to be @nonnull because that is the intent. But now IntelliJ prompts that config can never be null on line 188, so I am urged to remove it.

But if there is no explicit NN check then failure gets deferred to some later point that is a long way from the cause or intent.

@F43nd1r

F43nd1r commented Feb 17, 2016

Copy link
Copy Markdown
Member Author

If (and IMO only if) a method can be used as an entry point to the library, it is okay to double check (@ NonNull and if(param == null) ). For this case there is a suppress option. You can see it in use here.

I have already added that in the specific place you mentioned in my fork (https://github.com/F43nd1r/acra/blob/master/src/main/java/org/acra/ACRA.java#L189)

@F43nd1r F43nd1r mentioned this pull request Mar 13, 2016
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