4
\$\begingroup\$

This is a follow-up to the question that I posted earlier regarding my interpreter.

After a lot of help, I refactored my code and added more functionality to it. Now, it allows users to declare functions like so

func function_name param1 param2 param3 . / param3 * param1 param2 

or like this

func to_radians deg . * deg / 3.14159 180

and also like this

func sind theta . sin to_radians theta

As you can see, you would use a period (".") to tell the interpreter to stop taking in parameters and start reading the body of the function.

On top of that, it can read from source code.

Here's an example of such a source code:

; Variables
def PI * 4 atan 1

; Functions
func to_rad deg . * deg / PI 180

func sind theta . sin to_rad theta

; Run the program
sin to_rad 45
sind 45

Which will then output

0.707107

0.707107

If you have ever programmed using a Lisp-like programming language -- such as scheme -- you would notice that there are a lot of similarities with what I made. In fact, it was inspired by those programming languages.


Anyways, there's one limitation behind the code for the interpreter: when it encounters an error from within the user's source code, it's programmed to crash completely. That's not a problem. I did that on purpose. It's one way indicates to the user that there was an error.

Now, I want to implement a more appropriate error handler. But, before I do that, I would like to ensure that my code is easier to maintain.

I have a hunch that I should separate my code into multiple files. But that's just one, and even that I need help on how I should approach it.

What would be your suggestion?

#include <iostream>
#include <vector>
#include <string>
#include <fstream>
#include <sstream>
#include <cmath>
#include <cstdlib> // mmocny: I needed to add this to use atof
#include <functional>

using namespace std;

//----------------------------------

class Variable
{
public:
    Variable(const string& name, double val)
        : name_(name), val_(val) // mmocny: Use initializer lists
    {
    }

    // mmocny: get_* syntax is less common in C++ than in java etc.
    const string& name() const { return name_; } // mmocny: Don't mark as inline (they already are, and its premature optimization)
    double val() const { return val_; } // mmocny: Again, don't mark as inline
private:
    string name_; // mmocny: suggest renaming name_ or _name: Easier to spot member variables in method code, and no naming conflicts with methods
    double val_;
};

//----------------------------------

class Func
{
public:
    Func(const string& name, const vector<string>& params, const string& instr)
        : name_(name), params_(params), instr_(instr)
    {
    }

    const string& name() const { return name_; }
    const vector<string>& params() const { return params_; }
    const string& body() const { return instr_; }
private:
    string name_;
    vector<string> params_;
    string instr_;
};

//----------------------------------

// mmocny: Replace print_* methods with operator<< so that other (non cout) streams can be used.
// mmocny: Alternatively, define to_string()/str() methods which can also be piped out to different streams
std::ostream & operator<<(std::ostream & out, Variable const & v)
{
    return out << v.name() << ", " << v.val() << endl;
}

std::ostream & operator<<(std::ostream & out, Func const & f)
{
    out << "Name: " << f.name() << endl
        << "Params: " << endl;
    for (vector<string>::const_iterator it = f.params().begin(), end = f.params().end(); it != end; ++it)
    {
        out << "    " << *it << endl;
    }
    cout << endl;
    cout << "Body: " << f.body();
}

std::ostream & operator<<(std::ostream & out, vector<Variable> const & v)
{
    for (vector<Variable>::const_iterator it = v.begin(), end = v.end(); it != end; ++it ) // mmocny: Use iterators rather than index access
    {
        out << *it << endl;
    }
    return out;
}

std::ostream & operator<<(std::ostream & out, vector<Func> const & v)
{
    for (vector<Func>::const_iterator it = v.begin(), end = v.end(); it != end; ++it)
    {
        out << *it << endl;
    }
    return out;
}

//----------------------------------

class Interpreter
{
public:
    const vector<Variable>& variables() const { return variables_; }
    const vector<Func>& functions() const { return functions_; }

    // mmocny: replace istringstream with istream
    // mmocny: you only need to predeclare this one function

    double operate(const string& op, istream& in, vector<Variable>& v);
    double operate(const string& op, istream& in);

    double get_func_variable(const string& op, istream& in, vector<Variable> v)
    {
        // mmocny: instead of using a vector<Variable> you should be using a map/unordered_map<string,double> and doing a key lookup here
        for (int size = v.size(), i = size - 1; i >= 0; i--)
        {   
            if (op == v[i].name())
                return v[i].val();
        }

        for (int size = functions_.size(), i = 0; i < size; i++)
        {
            if (op == functions_[i].name())
            {
                vector<Variable> copy = v;
                vector<Variable> params;

                for (int size_p = functions_[i].params().size(), j = 0; j < size_p; j++)
                {
                    string temp;
                    in >> temp;
                    params.push_back(Variable(functions_[i].params()[j], operate(temp, in, v)));
                }

                /*for (vector<string>::const_iterator it = functions_[i].params().begin(),
                     end = functions_[i].params().end();
                     it != end; ++it)
                {
                    string temp;
                    in >> temp;
                    params.push_back(Variable(*it, operate(temp, in, v)));
                }*/

                for (int size_p = params.size(), j = 0; j < size_p; j++)
                {
                    copy.push_back(params[j]);
                }

                /*for (vector<Variable>::const_iterator it = params.begin(), end =  params.end();
                     it != end; ++it)
                {
                    copy.push_back(*it);
                }*/

                istringstream iss(functions_[i].body());
                string temp;
                iss >> temp;
                return operate(temp, iss, copy);
            }
        }

        // mmocny: what do you do if you don't find the variable?
        int char_to_int = op[0];
        cout << char_to_int << endl;
        cout << "'" << op << "' is not recognized." << endl;
        throw std::exception(); // mmocny: You should do something better than throw a generic exception()
    }

    double get_func_variable(const string& op, istream& in)
    {       
        return get_func_variable(op, in, variables_);
    }

    //----------------------------------

    bool is_number(const string& op)
    {
        // mmocny: someone else already mentioned: what if op is empty?
        int char_to_int = op[0];
        // mmocny: couple notes here:
        // 1) chars are actually numbers you can reference directly, and not need "magic" constants
        // 2) functions in the form "if (...) return true; else return false;" should just be reduced to "return (...);"
        // 3) is_number functionality already exists in libc as isdigit()
        // 4) long term, you are probably going to want to improve this function.. what about negative numbers? numbers in the form .02? etc..
        //return (char_to_int >= '0' && char_to_int <= '9');
        return isdigit(char_to_int);
    }

    //----------------------------------

    template< class Operator >
    double perform_action(istream& in, Operator op, vector<Variable>& v)
    {
        string left;
        in >> left;

        double result = operate(left, in, v); // mmocny: This is a big one: for correctness, you must calculate result of left BEFORE you read right

        string right;
        in >> right;

        return op(result, operate(right, in, v));
    }

    template< class Operator >
    double perform_action(istream& in, Operator op)
    {
        return perform_action(in, op, variables_);
    }

    //----------------------------------

    void define_new_var(istream& in)
    {
        string name;
        in >> name;

        string temp;
        in >> temp;

        double value = operate(temp, in);

        variables_.push_back(Variable(name, value));
    }

    //----------------------------------

    void define_new_func(istream& in)
    {
        string name;
        in >> name;

        string temp;
        vector<string> params;
        do
        {
            in >> temp;
            if (temp == ".")
                break;

            params.push_back(temp);

        } while (temp != ".");

        string body = "";
        while (in >> temp)
        {
            body += temp + " ";
        }

        Func fu(name, params, body);
        functions_.push_back(fu);
    }

private:
    vector<Variable> variables_;
    vector<Func> functions_;
};

//----------------------------------

double Interpreter::operate(const string& op, istream& in, vector<Variable>& v)
{
    double value;
    if (op == "+")
        value = perform_action(in, plus<double>(), v);
    else if (op == "-")
        value = perform_action(in, minus<double>(), v);
    else if(op == "*")
        value = perform_action(in, multiplies<double>(), v);
    else if (op == "/")
        value = perform_action(in, divides<double>(), v);
    /*else if (op == "%")
        value = perform_action(in, modulus<double>());*/
    else if (op == "sin")
    {
        string temp;
        in >> temp;
        value = sin(operate(temp, in, v));
    }
    else if (op == "cos")
    {
        string temp;
        in >> temp;
        value = cos(operate(temp, in, v));
    }
    else if (op == "tan")
    {
        string temp;
        in >> temp;
        value = tan(operate(temp, in, v));
    }
    else if (op == "asin")
    {
        string temp;
        in >> temp;
        value = asin(operate(temp, in, v));
    }
    else if (op == "acos")
    {
        string temp;
        in >> temp;
        value = acos(operate(temp, in, v));
    }
    else if (op == "atan")
    {
        string temp;
        in >> temp;
        value = atan(operate(temp, in, v));
    }
    else if (is_number(op))
        value = atof(op.c_str()); // mmocny: consider using boost::lexical_cast<>, or strtod (maybe)
    else
        value = get_func_variable(op, in, v);

    return value;
}

double Interpreter::operate(const string& op, istream& in)
{
    return operate(op, in, variables_);
}

//----------------------------------

void run_code(Interpreter& interpret, const string& op, istream& in)
{
    if (op == "def")
        interpret.define_new_var(in);
    else if (op == "func")
        interpret.define_new_func(in);
    else if (op[0] == ';' || op.empty())
        return;
    else
        cout << endl << interpret.operate(op, in) << endl;
}

//----------------------------------

bool is_all_blank(const string& line)
{
    for (int i = 0; i < line.size(); i++)
    {
        if (line[i] != ' ')
            return false;
    }

    return true;
}

//----------------------------------

int main()
{
    cout << endl << "LePN Programming Language" << endl;

    Interpreter interpret;

    while (cin)
    {
        cout << endl << "> ";

        string temp;
        getline(cin, temp);

        if (temp.empty()) // mmocny: This also handles the case of CTRL+D
            continue;

        istringstream iss(temp);
        string op;
        iss >> op;

        if (op == "quit")
            break;
        else if (op == "show_vars")
            std::cout << interpret.variables() << std::endl;
        else if (op == "show_func")
            std::cout << interpret.functions() << std::endl;
        else if (op == "open")
        {
            string filename;
            if (iss)
            {
                iss >> filename;
            }
            else
            {
                cin >> filename;
            }

            ifstream file(filename.c_str());

            while (file && !file.eof())
            {
                string line;
                getline(file, line);

                istringstream temp_stream(line);
                if (!temp_stream || is_all_blank(line))
                {
                    continue;
                }

                temp_stream >> op;
                int char_to_int = op[0];
                run_code(interpret, op, temp_stream);
            }
        }
        else
            run_code(interpret, op, iss);
    }
}
\$\endgroup\$
4
\$\begingroup\$

If you're going to get serious on this project, here are some questions worth considering:

  1. Are you using version control? We all have our preferences here but the important thing is you're using something. It gives you a way to hit 'UNDO' in case you make a big oopsie.
  2. Are there any unit tests? If you're currently not unit testing I would strongly suggest you start doing that now before your interpreter gets bigger. I like to use google test but there are many other C++ testing frameworks to choose from. Unit testing gives you a way to better control entropy in your project and this becomes more important as your project grows.
  3. Is the code well separated into different appropriate source files? Just like you wouldn't stuff every sentence into one gigantic paragraph in an essay, you don't want to stuff all your class, functions and variables into one source. In C++ it's customary to put each defined class in its own respective header(.h) and implementation(.cpp) file.

Now some specific comments regarding your presented code:

  • There is no clear separation between the lexing stage, parsing stage and processing in your design which is the usual expected approach when writing a translator, compiler or interpreter etc. It seems like these stages are mixed in together in an ad-hoc fashion inside your interpreter class. This coupling and lack of clear separation in your code will make it harder to maintain later when you add more stuff to it.
  • Typedef'ing std::vector usage to make it more container-agnostic. If you decide to change std::vector to a std::map later on you won't have to change it all over the place. Also consider using std::back_insert_iterator instead of directly std::vector::push_back to make it even more container-agnostic.
  • This function can be expressed more directly:

    bool is_all_blank(const string& line)
    {
        // Shouldn't it check for other possible whitespace too? Like tab for example
        return line.find_first_not_of( " \t" ) == string::npos;
    }
    
  • While skimming through your code I notice it's really bare of any comments. You might want to look into fixing that.

  • One more point, when reorganizing your code into multiple source files do not use any using namespace directives in the header files. Reason being you don't want this header to pollute the namespace of the components that includes this header.

\$\endgroup\$
  • \$\begingroup\$ You asked three questions. For the first one, yes, I do have a source control system; I use git. For the second one: no; in fact I have never done unit-tests in my life, and I think I should look up some tutorials on that. For the third question: no, it's not separated into files, but I was planning on doing that. \$\endgroup\$ – Sal May 5 '11 at 1:09
  • \$\begingroup\$ I'll echo the comment and unit test points. The unit tests will help you make sure you don't break things as you make changes. The comments will help you remember what you did when someone asks for a change 3 years from now. \$\endgroup\$ – jwernerny May 12 '11 at 21:42
2
\$\begingroup\$
  1. Some of the previous recommendation haven't yet been resolved.

  2. double get_func_variable(const string& op, istream& in, vector<Variable> v)

    • you generate exception in end of functions, which is ok, but where are you catching it?
    • how about std::find_if for this:

      for (int size = v.size(), i = size - 1; i >= 0; i--)
      {   
          if (op == v[i].name())
          return v[i].val();
      }
      
  3. For functions

    void define_new_var(istream& in)
    

    and

    void define_new_func(istream& in)
    

    check variable of functions for exist(using std::find_if, write common function).

  4. I think that the architecture will be expanded and may make sense to implement the command pattern and factory pattern:

    double Interpreter::operate(const string& op, istream& in, vector<Variable>& v)
    
  5. Use std::find_if for finding some objects in vector or list.

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