1
\$\begingroup\$
#include <iostream>
#include <vector>
#include <memory>


#define IS_DIGIT(x) x >= 48 && x <= 67 


static unsigned int allocated = 0;
void* operator new(size_t size) 
{
    allocated += size;
    void* mem = malloc(size);
    //std::cout << "allocating: " << mem << " with size: " << size << std::endl;
    return mem;

}
static unsigned int deleted = 0;
void operator delete(void* ptr, size_t size)
{
    deleted += size;
    //std::cout << "deleting: " << ptr << " with size: " << size << std::endl;
    free(ptr);
}

enum class Type 
{
    None, Number, Operator
};


std::ostream& operator<<(std::ostream& os, Type& type)
{
    switch (type)
    {
        case Type::None:
            os << "None";
            break;
        case Type::Number:
            os << "Number";
            break;
        case Type::Operator:
            os << "Operator";
            break;
        default:
            std::cout << "Unknown type for: " << &type << std::endl;
            break;
    }

    return os;
}
class Token 
{
public:
    Type type = Type::None;

    virtual void Fn() {};
};

class Number : public Token 
{
public:
    int value;

    Number(int _value)
        : value(_value) {
        type = Type::Number;
    }
};

class Operator : public Token 
{
public:
    Operator() 
    {
        type = Type::Operator;
    }

    virtual Number Operate(const Number& num1, const Number& num2) = 0;
};


class Plus : public Operator 
{
public:
    Number Operate(const Number& num1, const Number& num2) override
    {
        return Number(num1.value + num2.value);
    }
};

class Minus : public Operator 
{
public:
    Number Operate(const Number& num1, const Number& num2) override
    {
        return Number(num1.value - num2.value);
    }
};

void Tokenize(const std::string& input, std::vector<std::unique_ptr<Token>>& vec)
{
    std::string number;

    for (auto& c : input)
    {
        if (c == '\n' || c == ' ' || !c) continue;
        std::cout << "char: " << c << std::endl;
        if (IS_DIGIT(c)) number.push_back(c);

        else 
        {
            vec.push_back(std::move(std::make_unique<Number>(std::stoi(number.c_str()))));
            number.clear();

            switch (c)
            {
                case '+':
                    vec.push_back(std::move(std::make_unique<Plus>()));
                    break;
                case '-':
                    vec.push_back(std::move(std::make_unique<Minus>()));
                    break;
                default:
                    std::cout << "Unknown operator: " << c << std::endl;
                    break;

            }

        }
    }
    // for last digit
    if (!number.empty())
        vec.push_back(std::move(std::make_unique<Number>(std::stoi(number.c_str()))));
}

int main() 
{

// main scope
{
    std::vector<std::unique_ptr<Token>> tokens;
    Tokenize(" 78 + 1  *5-3", tokens);

    for (auto& i : tokens)
    {
        std::cout << "type: " << i.get()->type;
        Number* op = dynamic_cast<Number*>(i.get());
        if (op)
            std::cout << " num: " << op->value; 
        std::cout << std::endl;

    }
}

    std::cout << "total allocated: " << allocated << std::endl;
    std::cout << "total deleted: " << allocated << std::endl;

    return 0;
}

Can someone smart please check my code for inefficiencies and better optimizations ? Also I am not even sure if I do this right. thanks in advance

\$\endgroup\$
1
  • \$\begingroup\$ In parsing, a tokenizer takes a string of characters and outputs a list of tokens. You seem to be mixing the concepts of a tokenizer a parser and an interpreter. If you want to do that it's fine but then you will have to define the grammar for your calculator which I don't see. Having written my own interpreter in a haphazard manner and then subsequently applying proper computer science to refactor it, I can assure you that you will have much cleaner code once you define a context free grammar for this project. \$\endgroup\$ Commented Mar 16, 2023 at 20:45

3 Answers 3

3
\$\begingroup\$

As mentioned in the comments, you are perhaps confusing tokenizer, parser and interpreter, but I will review the code anyways.

Token Hierarchy

  1. Your Token class has two direct children i.e Number and Operator. This makes the type enum redundant and error-prone. The only reason you have a Type enum is to print the type, so why not just return a std::string instead?
  2. Your type variable is exposed and breaks encapsulation. Consider making it private and have a getType method in the class.
  3. Your Number class breaks Liskov's substitution principle. It has a public member variable value (which IMHO should be private with a public accessor) that is not present in your base class aka the token. This forces you to do a dynamic cast which is a code smell.
  4. If we look at the hierarchy carefully, neither Number nor Operator are tokens but it's the other way around. A token can be a Number or an Operator. Hence, a std::variant is better suited for this.
  5. If I had to make Token the root of the hierarchy and also keep the Type enum for some reason, I would change the implementation to be something like this:
{
public:

    virtual char const * const getType() = 0;
    virtual int evaluate() = 0;

private:
};

class Number : public Token
{
public:

    char const * const getType() override final
    { 
        return "Number";
    }

    int evaluate() override
    {
        return value;
    }
private:
    int value;
};

class Operator : public Token
{
public:

    char const * const getType() override final
    { 
        return "Operator";
    }

private:
};

class Plus : public Operator
{
public:

    int evaluate()
    {
        return operand1->evaluate() + operand2->evaluate();
    }

private:

    std::unique_ptr<Token> operand1;
    std::unique_ptr<Token> operand2;
};

Tokenize function

  1. Prefer returning values over "out" parameters; i.e. use the signature std::vector<std::unique_ptr<Token>> Tokenize(const std::string& input) instead.
\$\endgroup\$
1
\$\begingroup\$

General Observations

It isn't clear that this code is ready for code review since there are 2 operator functions (new and delete) that are never used.

I never expect to see malloc() or free() called in a C++. Since there aren't any calls to to new or delete it isn't completely clear what you were attempting to accomplish with this. The only value I see for any of this is if the code was being written on a system where you were needed to control memory usage such as on an embedded system, and then the only real memory allocation I would expect to see is where a block of memory was reserved for a local memory allocation scheme.

It isn't clear why the tokens are stored as pointers rather than just a vector of tokens. This is more of a C programming concept than a C++ programming concept. Since all token types are based on the token call the vector just needs to be a vector of token rather than pointers. This is how modern C++ is written, new and delete are generally avoided and not really necessary.

Group all the class definitions together prior to implementing a function unless the function is a member of a class. A general best practice in C++ is to implement each class as a header (.h file) and source file (.cpp file) pair. This type of modularity makes the code much easier to read and maintain. It isn't quite clear why this code appears together in the file:

enum class Type
{
    None, Number, Operator
};


std::ostream& operator<<(std::ostream& os, Type& type)
{
    switch (type)
    {
    case Type::None:
        os << "None";
        break;
    case Type::Number:
        os << "Number";
        break;
    case Type::Operator:
        os << "Operator";
        break;
    default:
        std::cout << "Unknown type for: " << &type << std::endl;
        break;
    }

    return os;
}

Portability

Not all systems use ASCII, rather than writing your own IS_DIGIT(x) it would be better to include the C library header ctype.h and use isdigit(). When you are using C++ this can be included by #include <cctype>.

Avoid Global Variables

There are 2 global variables (allocated and deleted). The fact that they are declared as static and limited to the file mitigates the problem somewhat. Since the new and delete operators are never used these global variables are not used.

It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.

Missing C++ Header

There is no #include <string>. This code does not compile using compilers that follow the C++ standard.

Prefer Braces { and } Around Single Statements in if or loops

Some programmers consider this a style issue, but it makes it much easier to read and maintain the code if each in an if, else or loop block is embedded within braces. Extending the functionality of these statements can be problematic when the braces are not used. For a more in depth discussion of this see the first 2 answers on this Stack Overflow question. As one of the answers points out this is true in all C like languages (C, C++, C#, JavaScript, Java, etc.). I have worked at multiple companies where this was required in the coding standard and flagged during code reviews.

\$\endgroup\$
1
\$\begingroup\$

Take care with macros. If IS_DIGIT() is expanded next to an arithmetic operator, you'll get surprising results, as it's not like a function call.

#define IS_DIGIT(x) ((x) >= 48 && (x) <= 67)

It's still not a good macro, as it expands x more than once, which can cause its side-effects to be repeated. Make it a function instead - or just use std::isdigit(), which seamlessly adapts to the execution character set instead of assuming input is always ASCII.


We shouldn't be declaring our own new and delete. If we find we ever need to, we should implement new correctly:

if (!mem) throw std::bad_alloc();

And we need to include <cstdlib> to declare std::malloc() and std::free().


Error messages should go to the error stream, not the output stream:

        default:
            std::cerr << "Unknown type for: " << &type << '\n';

Token being a base class, should have a virtual destructor.

It's probably a good idea to declare Token::Fn() = 0 (with or without a body) to make it pure abstract. Or just remove it, since it seems never to be used.


Number constructor should be explicit.


Good use of override where appropriate.


Missing include of <string>.


This looks like logging:

        std::cout << "char: " << c << std::endl;

So it should go to std::clog instead.


std::move() is a no-op when applied to rvalues, so not useful where it's called in Tokenise(). And we're missing its include (<utility>).


If we don't intend to modify a value, use const:

    for (auto const& i : tokens)

Consider using a std::shared_ptr so that we can use dynamic_pointer_cast() instead of dynamic_cast(get()).

\$\endgroup\$
2
  • \$\begingroup\$ Why use pointers at all? \$\endgroup\$
    – pacmaninbw
    Commented Mar 16, 2023 at 15:00
  • \$\begingroup\$ Perhaps that's necessary, due to the use of virtual functions. Might warrant a deeper look, but I filled a screen and then had to move to actual work. \$\endgroup\$ Commented Mar 16, 2023 at 15:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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