5
\$\begingroup\$

I'm making an app that creates pdfs of images layed out in a grid. I know the size of each grid element and how many fit on a page, it works out to a 5x6 grid. The syntax is not the real focus, but the logic for how I determine when to start a new row or page.

const pageconstants = {
 grid:{
   x: 5,
   y: 4,
 }
}

const griditems = []//an array containing 35 elements
const doc = new PDFDocument(
    {
        layout : 'portrait',
        width: 612,
        height: 792,
        autoFirstPage: false
    }
);

let gridx = 0;
let gridy = 0;
let pagenum = 1;

createNewPage(doc);//create new page and elements like header and footer

griditems.forEach((item) => {
    var x = 100 * gridx;//The actual equation has more parts, I'm simplifying here
    var y = 100 * gridy;

    addGridItem(doc, { x: x, y: y }, item)//places the image on the grid

    if( gridx == pageconstants.grid.x ){
      gridx = 0;

      if( gridy == pageconstants.grid.y ){
        subjecty = 0;
        pagenum++;
        createNewPage(doc)
      }
      else{
        subjecty++;
      }         
    }
    else{
      subjectx++;
    }
})

So basically, it checks if the current gridx has reached the max of 5 (6 because it starts at zero), and if so, resets gridx, and increments gridy to start the next row.

If gridy reaches it's maximum, gridx and gridy are reset and a new page is created again, so it can go again until there are no items left. This all works, but it feels really word, I really hate having nested if's like this, and incrementing all these numbers. Is there a more elegant way to achieve this?

\$\endgroup\$
3
\$\begingroup\$

So essentially, you want to layout an array of items in a 2D grid. This problem can be solved by using division and remainder operations to calculate the coordinates.

  • x is the remainder of dividing the index by maxX
  • y is the remainder of dividing by maxY the result of dividing the index by maxX.
  • A new page made is when both x and y are 0

Here's a rough sketch of how it looks:

let i = item index
let maxX = 5
let maxY = 6
let x = i % maxX 
let y = (i / maxX) % maxY 
let p = !(i % (maxX * maxY))

i: 0 1 2 3 4 5 6 7 8 9 ... 29 30 31 32 33 34 35
x: 0 1 2 3 4 0 1 2 3 4 ... 4  0  1  2  3  4  0
y: 0 0 0 0 0 1 1 1 1 1 ... 5  0  0  0  0  0  1
p: 1 0 0 0 0 0 0 0 0 0 ... 0  1  0  0  0  0  0

In code, this would be:

const griditems = []

const pageconstants = {
 grid:{
   x: 5, // 5 across
   y: 6, // 6 down
 }
}

const doc = new PDFDocument({
  layout : 'portrait',
  width: 612,
  height: 792,
  autoFirstPage: false
})

const getX = i => i % pageconstants.grid.x
const getY = i => Math.floor((i / pageconstants.grid.x) % pageconstants.grid.y)
const isNewPage = i => !(i % (pageconstants.grid.x * pageconstants.grid.y))

const positions = griditems.forEach((item, index) => {
  const x = 100 * getX(index)
  const y = 100 * getY(index)

  if (isNewPage(index)) createNewPage(doc)

  addGridItem(doc, { x, y }, item)
})
\$\endgroup\$
1
  • \$\begingroup\$ Wow thats pretty slick. thanks for the explanation. \$\endgroup\$ Oct 1 '19 at 17:49
2
\$\begingroup\$

You state "The syntax is not the real focus..." but I can not ignore style and syntax because they are the most important parts of any good code.

Style

  • More use of const. For example the line var x = 100 * gridx; should be const x = 100 * gridx;

  • Use property shorthand when creating objects. For example defining the object literal { x: x, y: y } can be shorthanded with {x, y}

  • Use the JavaScript layout conventions.

    • Spaces if( as if (, also if () {, } else {
    • No space if ( foo as if (foo and at start and end of object {x: foo, y: bar}
    • else on same line as closing } eg } else {
  • Use camelCase consistently. e.g. pageconstants to pageConstants and gridItems, gridX, pageNum, subjectY and so on. If you mix them you end up always having to look up the variable as humans do not include the capitalization in the mnemonic,

  • Be consistent in the style you use. E.g. you sometimes don't use semicolons to end a line and other times you do. Either do or do not. Spend some time investigating semicolon use to know the pros and cons.

  • Nested object and array opening lines are more readable if opened on the same line. e.g. foo(\n\t{\n\t\tbar: 0,\n\t}\n);\n as foo({\n\tbar: 0,\n});\n (CodeReview clarity is escape chars \n newline \t tag)

Code and logic

As pointed out in the answer Joseph you can use remainder to reduce the logic.

On top of that the need to floor the values can use a bitwise operation to reduce the verbosity of const foo = Math.floor(bar / width) to const foo = bar / width | 0; converts the result into a signed 32 bit integer. Note that you should only use this to floor values if foo is a positive and less than 2**31 - 1. Example 2**31 - 1 | 0 === 2147483647 while 2**31 | 0 1== 2147483648, it is in fact -2147483648

Rather than define the rows and columns of page items. It may be preferable to define the width and height of page items and then calculate the rows and columns from them. See example.

You have the property autoFirstPage as false in the doc. Yet the code does not consider this when it creates pages. If you change this value then you will end up with an empty page. Add some logic to determine if the first page needs to be created.

The same is true for the layout of the PDF (I am not sure if the doc swaps width and height if not portrait and guessing that they are not swapped in the example.

You are generating x and y coordinates but it is unclear if they are relative to the page, or relative to some centering construct and thus relative to a top left coordinate. In the example I have added a top left to the calculations that will auto center the grid coordinates. In effect the coordinates are relative to the page top left corner.

Your code is obviously not the code you use, so I don't know if you have it as part of a function, or it is actually flat code in the global names space. If it is flat global code it should be wrapped in a function to encapsulate it, or use modules. See example of IIF to encapsulate the scope.

Example

The example is somewhat longer than your original, however is is more flexible and less prone to layout error if the document setup is changed.

Note...

  1. that I freeze the objects created for layout. I have come to consider this as standard practice as it forces better object encapsulation code, and prevents object state mutation. It is a bit wordy but if you use it you can add an alias to the global space.

  2. that the function layout.grid.indexPos takes a second optional argument pos that defaults to an empty object. This is a general optimization and lets you reuse objects rather than create and deference an object every call. This technique (called pre allocation) can significantly reduce the memory management overhead. IE if you had a 1000 items, that is 999 less allocations and GC cleanups needed to process them all.

  3. I have made some name changes doc to PDF and pageconstants to layout and x, y for grid size to rows and cols (cols is columns using the common abbreviation)

  4. the line addGridItem(doc, {...pos.coord}, items[i++]); may be written without the need to copy the object pos.coord. If the function addGridItems makes a copy of the object or transfers the properties then use addGridItem(doc, pos.coord, items[i++]);

The code

;(() => {  // to encapsulate the scope away from the global scope.
    "use strict";
    
    const PDF = new PDFDocument( {
        layout : 'portrait',
        width: 612,
        height: 792,
        autoFirstPage: false,
    });

    const gridItems = [];
    const AUTO_CENTER_GRID_ITEMS = true;
    const ITEM_WIDTH = 120;
    const ITEM_HEIGHT = 200;
    const PAGE_WIDTH = PDF.layout === 'portrait' ? PDF.width : PDF.height;
    const PAGE_HEIGHT = PDF.layout === 'portrait' ? PDF.height : PDF.width;

    const layout = Object.freeze({
        grid: Object.freeze({
            cols: PAGE_WIDTH / ITEM_WIDTH | 0,
            rows: PAGE_HEIGHT / ITEM_HEIGHT | 0,
            left: (PAGE_WIDTH - (PAGE_WIDTH / ITEM_WIDTH | 0) * ITEM_WIDTH) / 2,
            top:  (PAGE_HEIGHT - (PAGE_HEIGHT / ITEM_HEIGHT | 0) * ITEM_HEIGHT) / 2,           
            indexPos(idx, pos = {}) {
                const grid = layout.grid;
                const c = pos.coord = pos.coord ? pos.coord : {};
                const col = idx % grid.cols;
                const row = idx / grid.cols | 0;
                pos.page = row / grid.rows | 0;                   
                c.x = col * ITEM_WIDTH;
                c.y = (row % grid.rows) * ITEM_HEIGHT;
                pos.newPage = !c.x && !c.y;
                if (AUTO_CENTER_GRID_ITEMS) {
                    c.x += grid.left;
                    c.y += grid.top;
                }
                return pos;
            },
        }),
    });

    function addGridedItems(doc, items) {
        const autoP = doc.autoFirstPage;
        var i = 0, pos;
        while (i < items.length) {
            pos = layout.grid.indexPos(i, pos);
            if (pos.newPage && (!autoP || (autoP && pos.page))) { createNewPage(doc) }
            addGridItem(doc, {...pos.coord}, items[i++]); 
        }
    }
    
    addGridItems(PDF, gridItems);
})();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.