4
\$\begingroup\$

I wanted to make a calculator in python so I first wrote a tokenizer. I have written one before but this time I tried to refine it a bit. Any thoughts on improvements, things I could of done better.

import re

class KeyWord:
    def __init__(self, name, regex):
        self.name = name
        self.regex = regex

class NewToken:
    def __init__(self, name, value, start, end):
        self.name = name
        self.value = value
        self.start = start
        self.end = end

class Lexer:
    def __init__(self):
        self.text = ""
        self.keyWords = []
        self.delimiters = ["+", "-", "/", "*", "%", "(", ")", "\n", " "]
        self.ignore = [" "]
        self.newTokens = []
        self.setTokens()

    def setTokens(self):
        self.keyWords.append(KeyWord("NUMBER", re.compile("([0-9]*\.[0-9]+)|([0-9]+\.[0-9]*)|([0-9])")))
        self.keyWords.append(KeyWord("PLUS", re.compile("\+")))
        self.keyWords.append(KeyWord("MINUS", re.compile("-")))
        self.keyWords.append(KeyWord("TIMES", re.compile("\*")))
        self.keyWords.append(KeyWord("DIVIDE", re.compile("\/")))
        self.keyWords.append(KeyWord("MODULO", re.compile("%")))
        self.keyWords.append(KeyWord("OPENBRACKET", re.compile("\(")))
        self.keyWords.append(KeyWord("CLOSEBRACKET", re.compile("\)")))

    def setText(self, text):
        self.text = text.strip() + "\n"

    def getTokens(self):
        self.newTokens = []
        word = ""

        #Loop through input
        for i in range(0, len(self.text)):

            ignoreFound = False
            for ig in self.ignore:
                if self.text[i] == ig:
                    ignoreFound = True

            tokenFound = False
            #Look for a delimiter
            for d in self.delimiters:
                if tokenFound:
                    break
                #If a delimiter is found
                if self.text[i] == d:
                    #Look for keyword
                    for t in self.keyWords:
                        match = t.regex.match(word)
                        if match:
                            self.newTokens.append(NewToken(t.name, word, (i - len(word)), i))
                            word = ""
                            tokenFound = True
                            break
                    #Check if delimiter has a token
                    if not ignoreFound:
                        for t in self.keyWords:
                            match = t.regex.match(d)
                            if match:
                                self.newTokens.append(NewToken(t.name, d, i, i))
                                tokenFound = True
                                break
            if not tokenFound and not ignoreFound:
                word += self.text[i]

        self.newTokens.append(NewToken("EOF", "", i, i))
        return self.newTokens
```
\$\endgroup\$
1
\$\begingroup\$

I have written one before

Yeah, I can tell. Nice. This looks well organized.

def setTokens(self):

PEP-8 asks that you spell it set_tokens. Similarly for some other setters and getters, and for assignments to e.g. self.key_words & self.new_tokens.

    self.keyWords.append(KeyWord("NUMBER", re.compile("([0-9]*\.[0-9]+)|([0-9]+\.[0-9]*)|([0-9])")))

Hmmm, several remarks.

DRY, you have an opportunity here to loop over a list of pairs (list of tuples), so there's just a single .append that we repeatedly call.

Perhaps you have your reasons, but I personally disagree with your definition of NUMBER. Choose a different name if it is a specialized restricted number from some problem domain. With alternation you mention frac|real|digit. The digit seems superfluous, it is subsumed by at least one of the other two. I'd prefer to see the order real|frac so we can mandate "starts with at least one digit". After that, you passed up the opportunity to say \.? for optional decimal. The frac case would then be "starts with decimal point". Also your current expression rejects 12 while accepting 1 and 123..

Rather than e.g. "[0-9]", consider saying r"\d".

    self.keyWords.append(KeyWord("PLUS", re.compile("\+"))) ...
    self.keyWords.append(KeyWord("DIVIDE", re.compile("\/")))

Please run flake8 against your code, and heed its warnings. Here, I have a strong preference for phrasing it re.compile(r"\+"), with a raw string, to avoid confusion with e.g. "\t\n" escapes. Also, the regex / works fine, similar to the regex Z, it is just a single character, no need for a \ backwhack.

    for i in range(0, len(self.text)):

Typical idiom would be for i, ch in enumerate(self.text).

The whole ig loop is much too verbose. Just test if ch in self.ignore (if self.text[i] in self.ignore) and be done with it.


Two algorithmic remarks:

It's not yet obvious to me why we need flag + loop to ignore optional whitespace. Wouldn't a simple continue at top of loop suffice? Maybe that range is not convenient, and you'd be happier with a while loop where you increment i yourself.

DRY, I'm not keen on self.delimiters, it is redundant with those beautiful regexes you went to the trouble of defining. I'd like to see one or the other of them go, so you don't have to remember to maintain two things in parallel when you (or someone else!) is maintaining this a few months from now.

Overall, looks pretty good.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback, i'll have a go at implementing your suggestions \$\endgroup\$ – Sheep_Wizard Jun 15 '20 at 12:05

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.