Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

edited May 23, 2017 at 11:33

Use the initializer list of your constructor, that would also elide the necessity of the "this->" see here here.

Later you will always set rawCount to 1 after the construction, so why not here? Your weight would also be better initialized with 1.0f. This would result in:

 template <typename T>
 Transition<T>::Transition(std::shared_ptr<State<T>> nextState) :
 weight(1.0f),
 rawCount(1),
 nextSate(nextState)
 {
 /* nothing */
 }

I would suggest to make the "transitions" member a std::vector. All you do is push_back and iterate over it. Since you asked specifically for performance: A vector stores its data in continous memory, a list not neccessraily, thus iterating over a vector will (general) be faster. See this question this question for detailed comparison.
If you want to only increment a variable use the prefix operator. For example this newState->rawStartCount++; should be changed to ++(newState->rawStartCount);. The brackets add a little clarity. The reason for that is discussed in many Q/A, for example here here. One reason is that the postfix might be less efficient because it needs to store a temporal copy of the value. Another reason is clarity: If you want to increment a value just do it and do nothing extra. Or better: Using the postfix can always lead to someone wondering if you need the old value.
const-correctness in range-base for-loops. If you do not change the state of the elements make them const. So this
```
 for(auto&& transition : transitions) {
 weightCounter += transition.rawCount;
 }
```

const correctnes in general. I do not see a single const in your code. But there are variables and functions which should be const, for example the numbers in your main function. Always try to make your code as const correct as you can do it.
use static_cast for number conversions. In your main function you do it but in MarkovChain<T>::calculateWeights() you use float(state->rawStartCount) / float(startWeightCounter);. This should also be a static_cast. One reason is that you can better look it up with search-functions. And again it expresses clearly what you do which is a "static cast". Again more detailed discussion is here here.
Replace std::endl with "\n" when appropriate. std::endl not only inserts a newline it also flushes the stream. This is in most situations not neccessary and can also lead to a huge performance impact. huge performance impact.
Delete not reachable code. In getNextSate(float), the output after the return is useless.

The state for the data added last to the chain will not have any transitions, thus getNextState(float) will crash.
As for your return "will never be called" I would suggest to use assertions. These can be usefull to tell everyone who reads the code what preliminaries you expect. For example in getNextState: assert(randomValue <= 1.0 + 1e-06) will show that you expect the randomValue to be lower than 1.0. I inserted the + 1e-06 cause of problems with comparing floating points comparing floating points. You can insert the same for randomValue >= 0.

Use the initializer list of your constructor, that would also elide the necessity of the "this->" see here.

Later you will always set rawCount to 1 after the construction, so why not here? Your weight would also be better initialized with 1.0f. This would result in:

 template <typename T>
 Transition<T>::Transition(std::shared_ptr<State<T>> nextState) :
 weight(1.0f),
 rawCount(1),
 nextSate(nextState)
 {
 /* nothing */
 }

I would suggest to make the "transitions" member a std::vector. All you do is push_back and iterate over it. Since you asked specifically for performance: A vector stores its data in continous memory, a list not neccessraily, thus iterating over a vector will (general) be faster. See this question for detailed comparison.
If you want to only increment a variable use the prefix operator. For example this newState->rawStartCount++; should be changed to ++(newState->rawStartCount);. The brackets add a little clarity. The reason for that is discussed in many Q/A, for example here. One reason is that the postfix might be less efficient because it needs to store a temporal copy of the value. Another reason is clarity: If you want to increment a value just do it and do nothing extra. Or better: Using the postfix can always lead to someone wondering if you need the old value.
const-correctness in range-base for-loops. If you do not change the state of the elements make them const. So this
```
 for(auto&& transition : transitions) {
 weightCounter += transition.rawCount;
 }
```

const correctnes in general. I do not see a single const in your code. But there are variables and functions which should be const, for example the numbers in your main function. Always try to make your code as const correct as you can do it.
use static_cast for number conversions. In your main function you do it but in MarkovChain<T>::calculateWeights() you use float(state->rawStartCount) / float(startWeightCounter);. This should also be a static_cast. One reason is that you can better look it up with search-functions. And again it expresses clearly what you do which is a "static cast". Again more detailed discussion is here.
Replace std::endl with "\n" when appropriate. std::endl not only inserts a newline it also flushes the stream. This is in most situations not neccessary and can also lead to a huge performance impact.
Delete not reachable code. In getNextSate(float), the output after the return is useless.

The state for the data added last to the chain will not have any transitions, thus getNextState(float) will crash.
As for your return "will never be called" I would suggest to use assertions. These can be usefull to tell everyone who reads the code what preliminaries you expect. For example in getNextState: assert(randomValue <= 1.0 + 1e-06) will show that you expect the randomValue to be lower than 1.0. I inserted the + 1e-06 cause of problems with comparing floating points. You can insert the same for randomValue >= 0.

Use the initializer list of your constructor, that would also elide the necessity of the "this->" see here.

Later you will always set rawCount to 1 after the construction, so why not here? Your weight would also be better initialized with 1.0f. This would result in:

 template <typename T>
 Transition<T>::Transition(std::shared_ptr<State<T>> nextState) :
 weight(1.0f),
 rawCount(1),
 nextSate(nextState)
 {
 /* nothing */
 }

I would suggest to make the "transitions" member a std::vector. All you do is push_back and iterate over it. Since you asked specifically for performance: A vector stores its data in continous memory, a list not neccessraily, thus iterating over a vector will (general) be faster. See this question for detailed comparison.
If you want to only increment a variable use the prefix operator. For example this newState->rawStartCount++; should be changed to ++(newState->rawStartCount);. The brackets add a little clarity. The reason for that is discussed in many Q/A, for example here. One reason is that the postfix might be less efficient because it needs to store a temporal copy of the value. Another reason is clarity: If you want to increment a value just do it and do nothing extra. Or better: Using the postfix can always lead to someone wondering if you need the old value.
const-correctness in range-base for-loops. If you do not change the state of the elements make them const. So this
```
 for(auto&& transition : transitions) {
 weightCounter += transition.rawCount;
 }
```

const correctnes in general. I do not see a single const in your code. But there are variables and functions which should be const, for example the numbers in your main function. Always try to make your code as const correct as you can do it.
use static_cast for number conversions. In your main function you do it but in MarkovChain<T>::calculateWeights() you use float(state->rawStartCount) / float(startWeightCounter);. This should also be a static_cast. One reason is that you can better look it up with search-functions. And again it expresses clearly what you do which is a "static cast". Again more detailed discussion is here.
Replace std::endl with "\n" when appropriate. std::endl not only inserts a newline it also flushes the stream. This is in most situations not neccessary and can also lead to a huge performance impact.
Delete not reachable code. In getNextSate(float), the output after the return is useless.

The state for the data added last to the chain will not have any transitions, thus getNextState(float) will crash.
As for your return "will never be called" I would suggest to use assertions. These can be usefull to tell everyone who reads the code what preliminaries you expect. For example in getNextState: assert(randomValue <= 1.0 + 1e-06) will show that you expect the randomValue to be lower than 1.0. I inserted the + 1e-06 cause of problems with comparing floating points. You can insert the same for randomValue >= 0.

added 1 character in body

Source Link

edited Dec 13, 2016 at 9:58

ab.o2c

edited Dec 13, 2016 at 9:58

ab.o2c

And last but not Leastleast Bugs:

The state for the data added last to the chain will not have any transitions, thus getNextState(float) will crash.
As for youyour return "will never be called" I would suggest to use assertions. These can be usefull to tell everyone who reads the code what preliminaries you expect. For example in getNextState: assert(randomValue <= 1.0 + 1e-06) will show that you expect the randomValue to be lower than 1.0. I inserted the + 1e-06 cause of problems with comparing floatingpointsfloating points. You can insert the same for randomValue >= 0.

And last but not Least Bugs:

The state for the data added last to the chain will not have any transitions, thus getNextState(float) will crash.
As for you return "will never be called" I would suggest to use assertions. These can be usefull to tell everyone who reads the code what preliminaries you expect. For example in getNextState: assert(randomValue <= 1.0 + 1e-06) will show that you expect the randomValue to be lower than 1.0. I inserted the + 1e-06 cause of problems with comparing floatingpoints. You can insert the same for randomValue >= 0.

And last but not least Bugs:

The state for the data added last to the chain will not have any transitions, thus getNextState(float) will crash.
As for your return "will never be called" I would suggest to use assertions. These can be usefull to tell everyone who reads the code what preliminaries you expect. For example in getNextState: assert(randomValue <= 1.0 + 1e-06) will show that you expect the randomValue to be lower than 1.0. I inserted the + 1e-06 cause of problems with comparing floating points. You can insert the same for randomValue >= 0.

Source Link

answered Dec 13, 2016 at 8:59

ab.o2c

answered Dec 13, 2016 at 8:59

ab.o2c

On your general coding style:

First the little things:

template <typename T>
Transition<T>::Transition(std::shared_ptr<State<T>> nextState) {
 weight = 0;
 rawCount = 0;
 this->nextState = nextState;
}

Use the initializer list of your constructor, that would also elide the necessity of the "this->" see here.

Later you will always set rawCount to 1 after the construction, so why not here? Your weight would also be better initialized with 1.0f. This would result in:

 template <typename T>
 Transition<T>::Transition(std::shared_ptr<State<T>> nextState) :
 weight(1.0f),
 rawCount(1),
 nextSate(nextState)
 {
 /* nothing */
 }

I would suggest to make the "transitions" member a std::vector. All you do is push_back and iterate over it. Since you asked specifically for performance: A vector stores its data in continous memory, a list not neccessraily, thus iterating over a vector will (general) be faster. See this question for detailed comparison.
If you want to only increment a variable use the prefix operator. For example this newState->rawStartCount++; should be changed to ++(newState->rawStartCount);. The brackets add a little clarity. The reason for that is discussed in many Q/A, for example here. One reason is that the postfix might be less efficient because it needs to store a temporal copy of the value. Another reason is clarity: If you want to increment a value just do it and do nothing extra. Or better: Using the postfix can always lead to someone wondering if you need the old value.
const-correctness in range-base for-loops. If you do not change the state of the elements make them const. So this
```
 for(auto&& transition : transitions) {
 weightCounter += transition.rawCount;
 }
```

should become

 for(const auto&& transition : transitions) {
 weightCounter += transition.rawCount;
 }

const correctnes in general. I do not see a single const in your code. But there are variables and functions which should be const, for example the numbers in your main function. Always try to make your code as const correct as you can do it.
use static_cast for number conversions. In your main function you do it but in MarkovChain<T>::calculateWeights() you use float(state->rawStartCount) / float(startWeightCounter);. This should also be a static_cast. One reason is that you can better look it up with search-functions. And again it expresses clearly what you do which is a "static cast". Again more detailed discussion is here.
Replace std::endl with "\n" when appropriate. std::endl not only inserts a newline it also flushes the stream. This is in most situations not neccessary and can also lead to a huge performance impact.
Delete not reachable code. In getNextSate(float), the output after the return is useless.

Now a few design related things:

Class State:

Why do you make some variables public and some private? If you want to use it as a "Storage" which has nothing more to do as to save the data then use a struct, otherwise I would advise to use a class and make every Member private. The purpose is that you have control over the access of the member and thus you can make your class "hard to use wrong" and if you you need permanent access to it from somewhere else maybe the member is misplaced. Example "startWeight" of the Class State is never used in the class itself. Maybe the Class MarkovChain shoukd have a struct which savaes a State and its startWeight.
"Make it hard to use wrong" : Your program depends on the fact that a user first adds the data, than calls calculateWights() and than iterates. Why not let the addData function update the Weights every time it is called? If you feel bad about the performance maybe you can build a addData function which takes a whole std::vector.
"A Function should have only one responsibility" : I would break your getDataAndAdvance(float) in one function getData() and one advance(float). This would make the code better to understand and the user could also operate multiple times on the same data before advancing.

And last but not Least Bugs:

The state for the data added last to the chain will not have any transitions, thus getNextState(float) will crash.
As for you return "will never be called" I would suggest to use assertions. These can be usefull to tell everyone who reads the code what preliminaries you expect. For example in getNextState: assert(randomValue <= 1.0 + 1e-06) will show that you expect the randomValue to be lower than 1.0. I inserted the + 1e-06 cause of problems with comparing floatingpoints. You can insert the same for randomValue >= 0.

I hope this helps a bit.

lang-cpp