2
\$\begingroup\$

I am a beginner in coding, writing my first bigger C++ project (aircraft initial sizing program). This code should be able to analyze the design in a probabilistic manner by using Monte Carlo Simulation. Hence, I have to generate random values for several independent random variables.

As I want to learn, how to program good code, I would be thankful for reviewing the simplified 'random number generation'-part of my code:

settings.h

#ifndef SETTINGS_H
#define SETTINGS_H

#include <iostream>
#include <string>
#include <vector>


class settings
{

public:
    struct distInput
    {
        std::string distName;
        double paramA; 
        double paramB;
        double paramC;
    };

    std::vector<distInput> distInputs;

    settings();
};

settings.cpp

#include "settings.h"


settings::settings()
{
    //Read input from file, here simplified
    distInput input1 = 
    {
        "Normal Distribution",
        100.,
        1.,
        0.
    };

    distInput input2 = 
    {
        "Triangular Distribution",
        80.,
        100.,
        109.
    };

    distInputs.push_back(input1);
    distInputs.push_back(input2);
}
//here are more functions

uncertainty.h

#ifndef UNCERTAINTY_H
#define UNCERTAINTY_H

#include <iostream>
#include <string>
#include <random>
#include <vector>


class uncertainty
{
public:
    void generateRN();

    std::vector<std::mt19937_64> randomNumberGenerators;
    std::vector<std::vector<double>> randomNumbers;
    settings *mySettingsPt;
    std::vector<settings::distInput> *inputData;

    uncertainty(settings &mySettings, double numbersRV);        
};

uncertainty.cpp

#include "uncertainty.h


uncertainty::uncertainty(settings &mySettings, double numbersRV)
    :
    mySettingsPt(&mySettings),
    inputData(&mySettingsPt->distInputs)
{
    for(unsigned int i = 0; i < numbersRV; ++i)
    {
        std::random_device rd;
        std::mt19937_64 mt_tmp(rd());
        randomNumberGenerators.push_back(mt_tmp);
        randomNumbers.resize(numbersRV);
    }    
}

void uncertainty::generateRN() 
{
    unsigned int numRV = inputData->size();

    for(unsigned int i = 0; i < numRV; ++i)
    {
        std::string name = inputData->at(i).distName;
        double a = inputData->at(i).paramA;
        double b = inputData->at(i).paramB;
        double c = inputData->at(i).paramC;

        if(name == "Normal Distribution")
        {
            std::normal_distribution<> dist(a, b);
            randomNumbers.at(i).push_back(dist(randomNumberGenerators.at(i)));
        }
        else if(name == "Triangular Distribution")
        {
            std::uniform_real_distribution<> dist(0, 1);
            double F = (b - a) / (c - a);
            double U = dist(randomNumberGenerators.at(i));
            if (U <= F)
                randomNumbers.at(i).push_back(a + sqrt(U * (c - a) * (b - a)));
            else
                randomNumbers.at(i).push_back(c - sqrt((1 - U) * (c - a) * (c - b)));
        }
    }
};

main.cpp

#include "settings.h"
#include "uncertainty.h"
#include <iostream>
#include <string>
#include <vector>
#include <random>


int main()
{
    // Initialize objects for computation
    settings mySettings;
    uncertainty myUncertainty(mySettings, mySettings.distInputs.size());

    // Start Computation
    for(unsigned int i = 0; i < 100; ++i)
    {
        myUncertainty.generateRN();
        // Other code, that uses the random values
    }
}

The code works fine, but I never had somebody reviewing my code skills. Improvement suggestions of any kind (How to structure .cpp and h. files, how to write that code more efficient, use of data structures and objects, etc.) are very welcome!

\$\endgroup\$
3
  • \$\begingroup\$ I really don't see where you set the seed for the random generator. Here I would add that you must setup the random seed that you will work with, and have it fixed for each experiment, that way, all your experiments are repeatable. If you take a "random" random seed, like the time, you wont have repeatability in your experiments and it gets more complicated in case you need to. \$\endgroup\$ – fernando.reyes Sep 23 '16 at 15:59
  • \$\begingroup\$ I seed the random generator in the 'uncertainty' constructor. If I understand you correctly, you suggest writing 'std::random_device rd; std::mt19937_64 mt_tmp(3456);' instead of 'std::random_device rd; std::mt19937_64 mt_tmp(rd());' for repeatability? \$\endgroup\$ – MarcJnm Sep 23 '16 at 16:07
  • \$\begingroup\$ If you are serious about experimentation and repeatibility. I mean, if you later need to pick up more data from an experiment and you need to repeat it. If that is not one of your needs, just ignore the comment. \$\endgroup\$ – fernando.reyes Sep 23 '16 at 16:11
2
\$\begingroup\$

You've clearly spent some time looking into best practices; your code is well separated, readable, logical. Keep up the formatting, and people will be happy to read over your code.

This did jump out at me though: you have two specific string literals "Normal Distribution" and "Triangular Distribution" that are repeated in separate files.. Advisable to avoid multiple instances of a string literal in future, especially in separate files; when code gets large, approaches like that can be time consuming and erroneous.

Think: constants and enums. Your compiler won't pick up on string literal typos like that, which could lead to irritating runtime errors or unexpected behaviour. On the other hand, it will of course alert you to undeclared (mistyped) handles in your code. Much more efficient bug removal.

\$\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.