888086 - sync/Utils.java claims `StringBuffer should be used instead`

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Kitching [:ckitching]]

Method byte2hex of Utils.java in mobile/android/base/sync has a comment reminding the programmer that StringBuffer should be used instead of String concatenation in this method, but concatenation is wastefully still being used (And what you really want here is a StringBuilder anyway, because it`s only being accessed from one thread)

Let´s fix that.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: byte2hex now uses StringBuilder, as requested by the comment.

A patch to do as described.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: byte2hex now uses StringBuilder, as requested by the comment, V2 (Added initial capacity).

Missed the initial capacity... fixed.

Comment 3

[:Margaret Leibovic]

Comment on attachment 768659 [details] [diff] [review]
byte2hex now uses StringBuilder, as requested by the comment, V2 (Added initial capacity).

This is also rnewman's domain.

You've made your way into sync code! :)

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 768659 [details] [diff] [review]
byte2hex now uses StringBuilder, as requested by the comment, V2 (Added initial capacity).

Review of attachment 768659 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to clean up the patch headers; 'hg qfold' concatenates your temporary commit messages into the destination patch.

In order to land this, you first need to take it upstream:

https://github.com/mozilla-services/android-sync/blob/develop/README.rst

This is a good set of skills to acquire. If you can test that change (there are plenty of tests in the repo that exercise it), open a pull req and we'll get it merged in both places.

If you're disinclined to do so (I know margaret is a harsh taskmistress), let me know and I'll take care of the landing.

Please don't set checkin-needed without addressing the style nit and upstreaming first.

::: mobile/android/base/sync/Utils.java
@@ +82,5 @@
>    /**
>     * Helper to convert a byte array to a hex-encoded string
>     */
>    public static String byte2hex(byte[] b) {
> +    StringBuilder hs = new StringBuilder(b.length*2);

Spaces around arithmetic operators.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I implemented the test ckitching was missing:

https://github.com/mozilla-services/android-sync/pull/362

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

r+ via github.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01c: m-c Patch

r+ via github.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/e72312830067

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e72312830067