1
\$\begingroup\$

I am studying design patterns in go. I need a review of this implementation. I started this exercise from wikipedia description of the pattern and implementing step by step all the sentences.

I decided to use two objects: the pool and the object managed by the pool.

type PoolObject struct {
    id int
}

type Pool struct {
    idle   *list.List
    active *list.List
}

When pool is initialized two empty lists were created.

func InitPool() Pool {
    pool := &Pool{
        list.New(),
        list.New(),
    }
    return *pool
}

Any time an object is requested, it's returned from the idle list or created from scratch.

func (p *Pool) Loan() PoolObject {
    if p.idle.Len() > 0 {
        for e, i := p.idle.Front(), 0; e != nil; e, i = e.Next(), i+1 {
            if i == 0 {
                object := e.Value.(PoolObject)
                return object
            }
        }
    }

    object := PoolObject{p.NumberOfObjectsInPool() + 1}
    p.active.PushBack(object)
    return object
}

Any time an object is returned, it's removed from the active list and pushed in the idle list.

func (p *Pool) Receive(object PoolObject) {
    p.idle.PushBack(object)
    for e, i := p.active.Front(), 0; e != nil; e, i = e.Next(), i+1 {
        if object == e.Value.(PoolObject) {
            p.active.Remove(e)
            return
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ In real code you'd use a sync.Pool (typically with tiny type safe wrappers around Pool.Get and Pool.Put for each specific pool type you use). \$\endgroup\$
    – Dave C
    Sep 24 '17 at 16:06
  • \$\begingroup\$ Go's container packages are rarely used in real code. Only use a list.List if you really need the properties of a doubly linked list, in the vast vast majority of cases a simple slice is much better and faster. In this case, you'd append items to an idle slice and pull off the tail end of it (i.e. use a slice as a LIFO queue; btw, a LIFO is preferable here vs FIFO both because it's easier to efficiently do the former with a slice and because recent objects are more likely to be in CPU/memory caches and should preferentially be reused first). \$\endgroup\$
    – Dave C
    Sep 24 '17 at 16:13
1
\$\begingroup\$

This loop doesn't loop:

for e, i := p.idle.Front(), 0; e != nil; e, i = e.Next(), i+1 {
    if i == 0 {
        object := e.Value.(PoolObject)
        return object
    }
}

On the first iteration, if there is a first iteration, it will return the first value from the pool. This should be written as a condition:

if e := p.idle.Front(); e != nil {
    return e.Value.(PoolObject)
}

The variable i is not used in this loop:

for e, i := p.active.Front(), 0; e != nil; e, i = e.Next(), i+1 {
    if object == e.Value.(PoolObject) {
        p.active.Remove(e)
        return
    }
}

It has no reason to be there, so it should be removed.

\$\endgroup\$
2
\$\begingroup\$
  1. You don't seem to have a policy for a maximum pool side. Is that by mistake or by design? What should happen if someone requests an object too many?

  2. Your choice of method names is slightly off. When I talk to an object pool, I don't want to Loan it an object, nor do I want to Receive an object. I want to "borrow," "obtain," "take" or "get" an object from the pool, and I want to "return" or "release" or "put" an object back into the pool.

    I'd suggest:

    obj = pool.Borrow()
    // ...
    pool.Return(obj)
    
  3. Finally, why does your object have an id? It seems unused. I'd suggest just using interface{} as your object type.

\$\endgroup\$
1
  • \$\begingroup\$ 1. good point. I'll improve the implementation. 2. Borrow is better also for me. Thank you. 3. Just for test purpose. Maybe is better to just check if objects are equals, instead of check their id. \$\endgroup\$
    – sensorario
    Sep 19 '17 at 14:35

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.