Review: C++ Algo - Style

Question

Can this code be reviewed?

I've updated this question to be about code style only, as all of its answered focused on this aspect. For the codes function, see Review: C++ Algo - Function

'algo' is an algorithm thats dynamic. 'algo' instances can be created, asked to become random, mutated and also run. They can be set to remember changes they make to themselves between runs or not. They also output values, these could be used for anything from value sequences such as mathematical number sequences to controls for a bot in a game. It's straightforward to specify limits on memory or computation steps for each as well and needless to say are entirely sandboxed.

By sandboxed I mean that they only compute and produce output as described, they cannot for example use local or global variables, print to the console, #include or write to files.

'algo's can be used where algorithms need to be portable and must be only able to calculate/compute. There is no distinction between data and instructions in an 'algo'.

A use is as values for directed search for algorithms such as with evolutionary algorithms, MCTS or others.

Another is in a data file that includes algorithms, like an image that includes its own way to decompress itself, that can therefore be constructed using the specific image that is to be decompresed.

They are delibratly general, being a component that could be used in many contexts and conceptually simple, as a number is.

// by Alan Tennant

#include <iostream>
#include <vector>
#include <string>
#include <time.h> // for rand

class algo
{
    public:
        std::vector<unsigned short> code;
        std::vector<unsigned int> output;
        bool debug_info;

        algo()
        {
            reset1(false);
            instructions_p = 11;
        }

        void random(unsigned int size)
        {
            code.clear();
            output.clear();
            for(unsigned int n = 0; n < size; n++) {
                code.push_back(rand() % instructions_p);}
            reset1(true);
        }

        void run(
            unsigned long most_run_time,
            unsigned int largest_program,
            unsigned int largest_output_size,
            bool reset)
        {
            if (reset && !is_reset_p)
            {
                reset1(true);
                output.clear();
                code2 = code;
                code_pos = 0;
            }
            is_reset_p = false;
            size_t code_size = code2.size();
            if (debug_info && !can_resume_p)
                std::cout<<"can't resume, reset first"<<std::endl;
            if(code_size == 0 || most_run_time == 0)
            {
                out_of_time_p = true;
                out_of_space_p = false;
                run_time_p = most_run_time;
            }
            else if (can_resume_p)
            {
                unsigned short instruction;
                bool cont = true;
                if(debug_info) {
                    std::cout<<"size: "<<code_size<<std::endl<<std::endl;}
                while(cont)
                {
                    instruction = code2[code_pos] % instructions_p;
                    if(debug_info) {std::cout<<code_pos<<", ";}
                    code_pos = (code_pos + 1) % code_size;
                    switch(instruction)
                    {
                        case 0:
                            if(debug_info) {std::cout<<"end";}
                            cont = false;
                            can_resume_p = false;
                        break;
                        case 1:
                            if(debug_info) {
                                std::cout<<"goto p1";}
                            code_pos = code2[(code_pos + 1) % code_size];
                        break;
                        case 2:
                            if(debug_info) {
                                std::cout<<"if value at p1 % 2 = 0 then goto p2";}
                            if(code2[code2[code_pos] % code_size] % 2 == 0) {
                                code_pos = code2[(code_pos + 1) % code_size];}
                            else {
                                code_pos += 2;}
                        break;
                        case 3:
                            if(debug_info) {std::cout<<"value at p1 = value at p2";}
                            code2[code2[code_pos] % code_size] =
                            code2[code2[(code_pos + 1) % code_size] % code_size];
                            code_pos += 2;
                        break;
                        case 4:
                            if(debug_info) {
                                std::cout<<"value at p1 = value at p2 + value at p3";}
                            code2[code2[code_pos] % code_size] = (
                            code2[code2[(code_pos + 1) % code_size] % code_size] +
                            code2[code2[(code_pos + 2) % code_size] % code_size]
                            ) % USHRT_MAX;
                            code_pos += 3;
                        break;
                        case 5:
                        {
                            if(debug_info)
                            {std::cout<<"value at p1 = value at p2 - value at p3";}
                            long v1 =
                            (long)code2[code2[(code_pos + 1) % code_size] % code_size] -
                            code2[code2[(code_pos + 2) % code_size] % code_size];
                            code2[code2[code_pos] % code_size] = abs(v1) % USHRT_MAX;
                            code_pos += 3;
                        }
                        break;
                        case 6:
                        {
                            if(debug_info) {std::cout<<"toggle value at p1";}
                            size_t v1 = code2[code_pos] % code_size;
                            unsigned short v2 = code2[v1];
                            if(v2 == 0) {code2[v1] = 1;}
                            else {code2[v1] = 0;}
                            code_pos++;
                        }
                        break;
                        case 7:
                            if(debug_info) {
                                std::cout<<"output value at p1";}
                        output.push_back(code2[code2[code_pos] % code_size]);
                            code_pos++;
                        break;
                        case 8:
                            if(debug_info) {std::cout<<"increase size";}
                            code2.push_back(0);
                        break;
                        case 9:
                        {
                            if(debug_info) {std::cout<<"increment value at p1";}
                            size_t v1 = code2[code_pos] % code_size;
                            code2[v1] = (code2[v1] + 1) % USHRT_MAX;
                            code_pos++;
                        }
                        break;
                        case 10:
                        {
                            if(debug_info) {std::cout<<"decrement value at p1";}
                            size_t v1 = code2[code_pos] % code_size;
                            code2[v1] = abs((code2[v1] - 1) % USHRT_MAX);
                            code_pos++;
                        }
                        break;
                    }
                    if(debug_info) {std::cout<<std::endl;}
                    run_time_p++;
                    code_size = code2.size();
                    code_pos = code_pos % code_size;
                    if(run_time_p == most_run_time) {
                        cont = false; out_of_time_p = true;}
                    if(code_size > largest_program)
                    {
                        cont = false;
                        can_resume_p = false;
                        out_of_space_p = true;
                        if (debug_info)
                            std::cout<<"became too large"<<std::endl;
                    }
                    if(output.size() > largest_output_size)
                    {
                        cont = false;
                        can_resume_p = false;
                        output.pop_back();
                        if (debug_info)
                            std::cout<<"too much output"<<std::endl;
                    }
                }
                if (debug_info)
                {
                    std::cout<<std::endl<<"size: "<<code_size<<std::endl<<
                        std::endl<<"output: "<<std::endl;
                    size_t output_size = output.size();
                    for (size_t t = 0; t < output_size; t++)
                        std::cout<<output[t]<<std::endl;
                }
            }
        }

        void mutate(unsigned int largest_program)
        {
            output.clear();
            size_t size;
            // special mutations
            while(rand() % 4 != 0) // 3/4 chance
            {
                size = code.size();
                if(rand() % 2 == 0) // 1/2 chance
                {
                    // a bit of code is added to the end (would prefer inserted anywhere)
                    if(size < largest_program) {
                        code.push_back(rand() % instructions_p);}
                }
                else
                {
                    // a bit of code is removed from the end (would prefer removed from anywhere)
                    if(size != 0)
                        code.pop_back();
                }
                // a section of code is moved, not yet implemented.
            }
            // mutate bits of the code
            size = code.size();
            if (size > 0)
            {
                unsigned int most_mutation = unsigned int(size * 0.1f);
                if(most_mutation < 9)
                    most_mutation = 8;
                unsigned int mutation = rand() % most_mutation;
                for(unsigned int n = 0; n < mutation; n++)
                    code[rand() % size] = rand() % instructions_p;
            }
            reset1(true);
        }

        #pragma region
        unsigned long run_time()
        {
            return run_time_p;
        }

        bool out_of_time()
        {
            return out_of_time_p;
        }

        bool out_of_space()
        {
            return out_of_space_p;
        }

        bool can_resume()
        {
            return can_resume_p;
        }

        bool is_reset()
        {
            return is_reset_p;
        }

        private:
            bool can_resume_p, is_reset_p,
                out_of_time_p, out_of_space_p;
            unsigned int code_pos;
            unsigned short instructions_p;
            unsigned long run_time_p;
            std::vector<unsigned short> code2;

            void reset1(bool say)
            {
                out_of_time_p = false;
                out_of_space_p = false;
                run_time_p = 0;
                code2 = code;
                code_pos = 0;
                can_resume_p = true;
                is_reset_p = true;
                if (say && debug_info)
                    std::cout<<"reset"<<std::endl;
            }
        #pragma endregion
};

void main()
{
    srand((unsigned int)time(NULL));
    algo a = algo();
    a.random(50);
    std::cout<<std::endl<<std::endl;
    a.run(10, 100, 10, false);
    std::cout<<std::endl<<std::endl;
    a.run(10, 100, 10, false);
}

I've improved the code in some of the ways described, above is the original code, thank you for the feedback.

Its usual for me to prioritise in this way, but I'm avoiding function calls in the main loop of run because I've found in this program they made a big difference to performance. Apart from that I like HostileFork and Anders K idea of using numeric_limits::max() over USHRT_MAX.

I'm not keen on "int main()", but in the interests of conformity its been changed from "void main()".

cstdlib and ctime have replaced time.h

From the answers, GCC doesn't seem that good, but to support it I have changed unsigned int(size * 0.1) to static_cast(size * 0.1).

Answer 1

For starters, code as written won't compile in GCC. :-(

"void main() is explicitly prohibited by the C++ standard and shouldn't be used"

http://stackoverflow.com/questions/204476/what-should-main-return-in-c-c

Rather than #include <time.h>, #include <cstdlib>. That will correctly give you rand and abs. There used to be some flak about including anything that ends in ".h" out of the standard library and instead use the "c-prefixed-and-non-suffixed" versions, but last I checked it didn't actually make a difference. Still, some people think it does, so best not to upset them.

Read this post of mine about numeric_limits, and replace your USHRT_MAX and UINT_MAX appropriately.

http://hostilefork.com/2009/03/31/modern_cpp_or_modern_art/

GCC doesn't like this line:

unsigned int most_mutation = unsigned int(size * 0.1);

I'm not sure what the point of that is supposed to be. If you want to static cast it for clarity, I guess that's fine:

unsigned int most_mutation = static_cast<unsigned int>(size * 0.1);

With those changes, it compiles in GCC. Speaking of which: whatever compiler you are using...it can be helpful to have a virtual machine around to use some different ones on your code (Clang, GCC, MSVC) and see what they report.

---

The next best-practices step is to bump the warnings all-the-way-up. I'm now using settings I got from an answer by David Stone. Here's what that gives us:

test.cpp:226:0: error: ignoring #pragma region  [-Werror=unknown-pragmas]
test.cpp:272:0: error: ignoring #pragma endregion  [-Werror=unknown-pragmas]
test.cpp: In member function ‘void algo::run(long unsigned int, unsigned int, unsigned int, bool)’:
test.cpp:107:86: error: use of old-style cast [-Werror=old-style-cast]
test.cpp:67:27: error: switch missing default case [-Werror=switch-default]
test.cpp: In member function ‘void algo::mutate(unsigned int)’:
test.cpp:212:40: error: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
test.cpp:215:77: error: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
test.cpp:218:50: error: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
test.cpp:220:35: error: conversion to ‘size_t {aka unsigned int}’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
test.cpp: In function ‘int main()’:
test.cpp:277:34: error: use of old-style cast [-Werror=old-style-cast]

You can address those on your own, but I'll cover the pragma philosophy. Firstly: I never indent preprocessor directives--it calls them out more clearly. But even better: don't use 'em, especially not for a fluffy IDE feature (why doesn't it use a certain comment to cue that?) Pragmas are "implementation defined", and putting little bits of dirt in your program like this is a slippery slope. GCC used to discourage this:

In some languages (including C), even the compiler is not bound to behave in a sensible manner once undefined behavior has been invoked. One instance of undefined behavior acting as an Easter egg is the behavior of early versions of the GCC C compiler when given a program containing the #pragma directive, which has implementation-defined behavior according to the C standard. In practice, many C implementations recognize, for example, #pragma once as a rough equivalent of #include guards — but GCC 1.17, upon finding a #pragma directive, would instead attempt to launch commonly distributed Unix games such as NetHack and Rogue, or start Emacs running a simulation of the Towers of Hanoi.

---

Now I'll just ramble about formatting and style, without trying to grok the "big picture" of what your code is for. :)

This is a matter of taste, but in implementation files using namespace std; can make your code less wordy. (Doing it in headers is not considered a good practice, as it would then be inherited by all implementation files that used the headers...giving them less control over potential name collisions.)

Also, it's generally considered a bad idea to make data members in your classes public. Narrowing the interface through methods gives you more wiggle room to modify the implementation without clients of the class to be rewritten. So:

using namespace std;

class algo
{
    private:
        vector<unsigned short> code;
        vector<unsigned int> output;
        bool debug_info;

    public:
        algo()
        {
        /* stuff... */

I personally like to see spaces between things and logical breaks in output. So following on that I'd turn:

std::cout<<std::endl<<"size: "<<code_size<<std::endl<<
        std::endl<<"output: "<<std::endl;
size_t output_size = output.size();
for (size_t t = 0; t < output_size; t++)
    std::cout<<output[t]<<std::endl;

...into the much-less-claustrophobic:

cout << endl;
cout << "size: " << code_size << endl;
cout << endl;
cout << "output: " << endl;
for (size_t t = 0; t < output_size; t++) {
    cout << output[t] << endl;
}

Again in the "matter of taste" department, I prefer to use the C++ keywords for logical operations, so I would write:

if (reset && !is_reset_p)

...as:

if (reset and (not is_reset_p))

But that's just me, since I never try to write C++ code that will still compile in C...so I figure the keywords are there, why not use 'em for readability. Plus I use a proportional font to edit, and the ! for not can be hard to see...e.g. !list.isNull().

Also, I indent the breaks in my switch statements and not the cases. It makes it more parallel to an "if" by putting the case at the same level as the switch, and makes the cases stand out better. You also don't chew up screen space as rapidly:

switch(instruction)
{
case 0:
    if (debug_info) {
        cout << "end";
    }
    cont = false;
    can_resume_p = false;
    break;
 case 1:

I also would suggest always putting braces around code in if statements, and not putting code on the same line as the condition. Beyond aesthetics, this makes editing the condition and editing the code edits to different lines in version control systems. (And always using braces means you don't have to generate a diff on the condition line when you go from one-line of code to multiple-lines of code or vice-versa.)

The use of unsigned types is verbose and of questionable advantage. I never really thought that reclaiming half the numeric range mattered much (most of the time). But once I believed it was better for quantities that were always supposed to be unsigned to be labeled thusly, for type correctness. It isn't really the win you'd think, and probably obscures as many bugs as it prevents.

I won't rehash the pros and cons here...I'll just say that if you want a real type-safe solution (e.g. in computer security code) you need something like SafeInt:

http://safeint.codeplex.com/

---

Regarding the big picture I won't invest that time. I'll just say that if you're trying to write generalized algorithms for C++, then be sure to spend a little while looking at <algorithm>:

http://en.cppreference.com/w/cpp/algorithm

It may provide inspiration for your design. Perhaps what you really want is a templated class with some kind of iteration interface?

You might consider asking a StackOverflow question that doesn't include your code above, but rather establishes a clear usage scenario. Then ask for suggestions on what methodology to use to meet the need. Your description is too vague, and suffers from the problem that you don't clearly define what it doesn't do...and while terms "sandboxed" might be meaningful in your mind it doesn't convey a clear requirement for this code to me.

Answer 2

My 2c:

---

#include <iostream>
#include <vector>
#include <string>
#include <time.h> 

use cpp version of headers i.e. cstdlib and ctime

---

in general split your class in a header and cpp, it makes it easier to get an overview of the contents of the class.

---

    algo()
    {
        reset1(false);
        instructions_p = 11;
    }

initialize your member variables in the ctor i.e algo() : debug_info(false) ... {} normally it is also a good habit to have a dtor, default copy ctor and assignment, if for nothing else to disallow them.

---

    void random(unsigned int size)
    {
        code.clear();
        output.clear();
        for(unsigned int n = 0; n < size; n++) {
            code.push_back(rand() % instructions_p);
        }
        reset1(true);
    }

i would like a bit more descriptive method names - random what?

---

    void run(
        unsigned long most_run_time,
        unsigned int largest_program,
        unsigned int largest_output_size,
        bool reset)
    {...

when you start to have many parameters it helps having comments to describe what they mean and what unit they are expecting (kb? bytes?). Also it is good to have an assert or two inside each method to catch any unexpected values a programmer may toss to your function.

---

        if (debug_info && !can_resume_p)
            std::cout<<"can't resume, reset first"<<std::endl;

arguable it may be a bit cleaner to have a debug output function to print the debug information so that you can redirect the output easier or have a stream member variable:

---

        if (debug_info && !can_resume_p)
            _err <<"can't resume, reset first"<<std::endl;

or wrap in a debug function which does nothing when debug_info is false:

---

        if (!can_resume_p)
          debug("can't resume, reset first");

---

switch statement:

                switch(instruction)
                {
                    case 0:

use an enum for constants to make case statement clearer:

                    case IncreaseSize:
                        debug("increase size");
                        code2.push_back(0);
                    break;

---

                        code2[v1] = (code2[v1] + 1) % USHRT_MAX;

use the std::numeric_limits (include climits) instead:

code2[v1] = (code2[v1] + 1) % numeric_limits<unsigned short>::max()

always have a default: case in the case statement to catch weird values

---

            if (debug_info)
            {
                std::cout<<std::endl<<"size: "<<code_size<<std::endl<<
                    std::endl<<"output: "<<std::endl;
                size_t output_size = output.size();
                for (size_t t = 0; t < output_size; t++)
                    std::cout<<output[t]<<std::endl;
            }

in the above it would be more clearer to move the code into a separate function which does nothing if debug_info is false

finally since you use index of vector a lot you want to use at(n) on the vector instead of [n] to catch if you go outside the index. at(n) will then throw an exception.