On Fri, 17 Jan 2020 14:39:14 GMT, Frederic Thevenet <github.com+7450507+ftheve...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1316: >> >>> 1315: } >>> 1316: } >>> 1317: } else { >> >> I would extract this code into its own method similar to `doSnapshotTile`: >> >> `assemble(scene, xMin, yMin, width, height, root, transform, depthBuffer, >> fill, camera, wimg, maxTextureSize);` >> >> (`assemble` is a bad name, I didn't think about a better one). >> >> The method can return he resulting `WritableImage`, but it is not needed >> since it is manipulated via "side-effects". I would, however, bring it line >> with the `else` clause - either both use `wimg = methodName(..., wimg, >> ...);` or just `methodName(..., wimg, ...);`. This is fine since the input >> `WritableImage` is never `null`. From a readability point of view, using >> return values seems better. > > I'm not 100% convinced this would really add much to the readability of the > code; I extracted the code from `doSnapshotTile` in its own method because it > is called twice (on both sides of the `if (height > maxTextureSize || width > > maxTextureSize)` condition, actually), but this isn't the case here. > I've got no strong feeling against it either, so I don't know; anybody else > care to comment? I also don't have a strong opinion, so I'm OK with you leaving it as-is. ------------- PR: https://git.openjdk.java.net/jfx/pull/68