Return table start PDPage from drawer #115
Conversation
| pageToDrawOn = pageSupplier.get(); | ||
| document.addPage(pageToDrawOn); | ||
| } else | ||
| pageToDrawOn = document.getPage(document.getNumberOfPages() - 1); | ||
| pageToDrawOn = document.getPage(document.getNumberOfPages() - 1); |
There was a problem hiding this comment.
Please add brackets around the bodies of if s and else s. This is a general recommendation. It's very easy to make mistakes on refactorings and it's pretty simple to prevent 😉
| @@ -139,11 +155,15 @@ public void draw(Supplier<PDDocument> documentSupplier, Supplier<PDPage> pageSup | |||
|
|
|||
| protected PDPage determinePageToDraw(int index, PDDocument document, Supplier<PDPage> pageSupplier) { | |||
There was a problem hiding this comment.
Is the returned value used at all? So either return the value and assign it where it is called (preferably) or just make the method void.
There was a problem hiding this comment.
The returned PDPage is used by the draw(Supplier documentSupplier, Supplier pageSupplier, float yOffset) method. Not sure I got your suggestion.
There was a problem hiding this comment.
Sorry, my bad. You are right. Still, there is a thing which I don't like about this method and which should be easy to fix: There is a side effect, namely setting the member variable tableStartPage within the method. At the same time this value is returned. Since it is called only once one could argue that it would make more sense to set the member variable after the method is called using its returned value.
(There is a 2nd side effect nonetheless though which is setting the member variable startTableInNewPage – but the fewer side effects the better in my opinion.)
There was a problem hiding this comment.
Point taken. Have moved both the if conditions into the parent draw method.
Attempt to solve issue 114. - #114.