I have module called printer allocator (PrinterAllocator) which will allocate the next available printer to the requester.

import java.util.ArrayList;
import java.util.Arrays;
import java.util.BitSet;
import java.util.List;

public class PrinterAllocator {

    private static final int TOTAL_AVAILABLE_PRINTERS = 25;
    private BitSet bitSet = new BitSet( TOTAL_AVAILABLE_PRINTERS );

    public PrinterAllocator() {
        List<Integer> unassignedPriters = new ArrayList<>( Arrays.asList( 1, 2, 3, 24 ) ); // but this array will be fetched from database
        unassignedPriters.forEach( printerId -> bitSet.set( printerId - 1 ) );

    public int getNextPrinter() {
        int index = bitSet.nextClearBit( 0 ) + 1;
        bitSet.set( index );
        return index + 1;

    public boolean hasEnoughPrinters(int printersNeeded){
        /* point 1*/
        return bitSet.cardinality()+printersNeeded<=TOTAL_AVAILABLE_PRINTERS;


Point 1: I am unable to replace 'point 1' with following code

 return bitSet.cardinality()+printersNeeded<=bitSet.size()


return bitSet.cardinality()+printersNeeded<=bitSet.length();

Because bitSet.size() and bitSet.length() has a different meaning that is size() != length() != TOTAL_AVAILABLE_PRINTERS

Any better Idea to implement PrinterAllocator with/without bitset

  • \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way! Please see our help center for more information. \$\endgroup\$ – Malachi Mar 15 '19 at 13:49

Why do you use BitSet? You're not saving kilobytes of memory by representing your printers as a set of bits. Go for code clarity and represent each printer with a dedicated instance of Printer class that holds the allocation status of the printer in question. Store them in a List or other data structure that suits your needs. You could even have two LinkedLists for allocated and free printers and just pop the first from the free list and push it to the end of the allocated list. You need to synchronize anyway. No need to count sizes to see if there are available printers. Just check isEmpty().

bitSet is a bad name for printer reservartion status. Java is a strongly typed language so you don't need to repeat the type in the field name. Use printerAllocationStatus if you really need to use a BitSet (see above).

You're stacking on responsibilities by implementing hasEnoughPrinters in your allocator. Instead implement getNumberOfFreePrinters and let the caller decide what to do with the information. The method is of limited use anyway, since printer allocation status might change right after the caller has checked for availability making the data the caller has completely useless.

getNextPrinter does not communicate the side effect of changing printer allocation status in it's name. Use allocatePrinter instead.

Use consistent terminology. In a PritnerAllocator a printer is allocated or free, not assigned.

| improve this answer | |

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