Change method name 'generate' to 'createJpeg', 'increase' to 'includeBitmap', 'ensure' to 'loadLibrary', 'build' to 'getImageRequest', 'get' to 'createContext'#2385
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
pdhung3012
changed the title
pdhung3012
changed the title
pdhung3012
changed the title
oprisnik
left a comment
oprisnik
left a comment
There was a problem hiding this comment.
Thanks for the pull request! Most of these renames do not improve the readability of the code. Please revert ones that are breaking API changes since we do not want to break existing usages.
| private static @Nullable Supplier<Boolean> sDebugOverlayEnabledSupplier; | ||
|
|
||
| public static synchronized FrescoContext get(Resources resources) { | ||
| public static synchronized FrescoContext createContext(Resources resources) { |
There was a problem hiding this comment.
This change doesn't make sense. It doesn't always create a new instance.
| } | ||
|
|
||
| public ImageUrlsRequest build() { | ||
| public ImageUrlsRequest getImageRequest() { |
There was a problem hiding this comment.
This doesn't make sense and is a huge breaking API change.
| * @return true if and only if bitmap is successfully included in the count | ||
| */ | ||
| public synchronized boolean increase(Bitmap bitmap) { | ||
| public synchronized boolean includeBitmap(Bitmap bitmap) { |
There was a problem hiding this comment.
The old name is clearer IMO.
There was a problem hiding this comment.
Thank you for your notification. Do you think that, the other case 'generate' to 'createJpeg' and 'ensure' to 'loadLibrary' improve the readability?
|
It's unclear that the proposed names are better. Closing as it has been stale for a long time. |
5 participants
The class EmptyJpegGenerator is used to generate an empty JPEG image. This method named "generate" is to create a new JPEG image. Thus, the method name "createJpeg" is more intuitive than "generate".
The class BitmapCounter is used to count the Bitmaps. This method named "increase" is to check the bitmap. Thus, the method name "includeBitmap" is more intuitive than "increase".
The class BitmapCounter is used to represent GifImage. This method named 'ensure' is to load library for image processing. Thus, the method name 'loadLibrary' is more intuitive than 'ensure'.
The class ImageUrlsRequestBuilder is used to build ImageUrlsRequest. This method named 'build' is to get a new Image url request. Thus, the method name 'getImageRequest' is more intuitive than 'build'.
The class DefaultFrescoContext is used to represent DefaultFrescoContext. This method named 'get' is to create a context. Thus, the method name 'createContext' is more precise than 'get'.