current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm trying to implement a template that takes a container as parameter. The template has a getnext method that cycles through the elements in the parameter. Take note that I'm not using C++11.

Header:

#pragma once

template<template<class, class> class TContainer, class TObject>
class cycle
{
public:

    explicit cycle( TContainer<TObject, std::allocator<TObject>> & container )
        : mContainer( container ), index(0) {}

    TObject getNext(int numObjectsToCycle) 
    { return mContainer[index++ % numObjectsToCycle]; }
private:
    TContainer<TObject, std::allocator<TObject>> & mContainer;
    int index;
};

Implementation:

#include <iostream>
#include <vector>

#include "cycle.h"

using namespace std;


class Processor
{
   int mIndex;
   vector<int> numbers;

public:
   cycle<vector, int> cycler;
   // Is it safe to pass numbers since it is declared first??
   Processor() : mIndex(0), cycler(numbers) {} 
   void update() { cout << numbers[mIndex++ % numbers.size()] << std::endl;}
   void addNumber(int x) { numbers.push_back(x); }
};


int main()
{
   Processor S;

   for (int i = 0; i < 5; ++i)
   {
      S.addNumber(i+1);
   }
   cout << "using update" << endl;
   for (int c = 0; c < 10; ++c)
   {
      S.update();
   }

   cout << "using cycle" << endl;
   for (int c = 0; c < 10; ++c)
   {
      cout << S.cycler.getNext(5) << endl;
   }


   std::cin.get();

}

Any improvement or potential issues to the code?

share|improve this question

3 Answers 3

up vote 3 down vote accepted

Here is how I would write it:

template<typename C>
class cycle
{
    C& mContainer;
    size_t index;
    using ref = decltype(mContainer[0]);

public:
    explicit cycle(C& container) :
        mContainer(container), index(0) {}

    ref getNext()
    {
        return mContainer[index++ % mContainer.size()];
    }
};

Notes:

  • The template parameter is the container, C. This not only simplifies code, but also works for containers that do not follow a specific pattern, e.g. have more parameters than just a value type and an allocator type.

  • The return type of getNext is automatically inferred from C. This is not just C's value_type but a (non-const) reference to it.

  • If I understand correctly from the title, C is supposed to be a sequence of containers? If so, the value_type is a container itself so it is even more important that a reference is returned from getNext.

  • Why pass a parameter to getNext? To cycle correctly, you should use mContainer.size(), exactly as you do in Processor::update, right?

  • index is of type size_t

With only a few changes, here is Processor:

class Processor
{
   size_t mIndex;
   std::vector<int> numbers;

public:
   cycle<std::vector<int>> cycler;
   Processor() : mIndex(0), numbers(), cycler(numbers) {}
   void update() { std::cout << numbers[mIndex++ % numbers.size()] << std::endl;}
   void addNumber(int x) { numbers.push_back(x); }
};

Notes:

  • again, mIndex is a size_t
  • cycler is a cycle<std::vector<int>>
  • numbers is default-initialized
  • cycler is safely constructed after numbers

Also, whenever you want to

using namespace std;

for convenience, only do it inside another (your own) namespace, never globally.

EDIT

I just noticed you're not using C++11. In this case, just use

    typedef typename C::value_type& ref;

instead.

share|improve this answer
    
The parameter to getNext allows me to limit the cycling to the first 5 out of 10 elements for example. So if I want only the 10 elements all the time, then I won't need a parameter. –  kunkka_71 Apr 25 '14 at 8:59
    
@kunkka_71 Ok, then maybe you could have two versions if you also need the automatic one. In this case, one overload could call the other. –  iavr Apr 25 '14 at 9:03
    
after using typedef as you've mentioned, using ref = decltype(mContainer[0]); does not compile. decltype is C++11 only? –  kunkka_71 Apr 25 '14 at 11:09
    
@kunkka_71 Yes, the last typedef ... line is a replacement for using ref = .... –  iavr Apr 25 '14 at 13:17
up vote 2 down vote

A few remarks:

  • Try to avoid using namespace std.

  • In Processor, you should also explicitly call the constructor of the vector.

  • Your cycle class could be templated on the container type only, and use TContainer::value_type instead.

share|improve this answer
up vote 2 down vote

A few things about update():

  • It's less readable and maintainable to cram the functionality into one line. Just declare it within the class and define it outside. If the compiler decides that it should be inlined, it will do that for you.

  • Prefer "\n" over std::endl when just outputting a newline. The latter also flushes the buffer, which is slower and unneeded here. See this for more information.

  • update is not a very accurate name. What exactly does it update?

share|improve this answer
    
It's actually a stripped down code so the contents of update is not just that. And the couts are really just to output them on the screen. –  kunkka_71 Apr 25 '14 at 8:28

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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