Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have been teaching myself Python 3. I watched a video on YouTube entitled "stop writing classes". The Gtable class doesn't even have an init function, so I can't help but wonder if I should have done something different. Also as someone teaching themselves a language for the first time I would really appreciate any critique on how to write the code more efficiently, as well as any anti-idioms unintentionally used. Specifically I feel like there was probably a better way to generate the dictionaries, but especially with the first one I was having trouble writing a comprehension.

##input file text
##|HAAS T|TARGET T|XGEOM|ZGEOM
##$T0101$T0101$X22.960$Z20.0$
##$T0202$T0606$X32.425$Z7.376$
##$T0303$T0404$X30.588$Z15.620$
##$T0404$T0202$X22.367$Z18.549$
##$T0505$T0808$X33.291$Z8.197$

class GeometryTable(dict):
    "create a geometry table from a file for use in regexes"
    def __init__(self):
        self.d={}
    def create(self,location):
        f=open(location,'r')
        tablestring=f.read()
        f.close()
        tablelist=[]
        tablelist=tablestring.split('$')
        #delete unecessary data from list
        tablelist.pop(0)
        tablelist=[item for item in tablelist if item!='\n']
        #create a dictionary
        count=0
        keylist=[]
        valuelist=[]
        while count<len(tablelist):
            keylist.append(tablelist[count])
            valuelist.append([tablelist[count+1],tablelist[count+2],tablelist[count+3]])
            count+=4
        self.d=dict(zip(keylist,valuelist))
    def xval(self,string):
        if string in self.d:
            return (self.d[string][1])
        else:return(r"X20.0")
    def zval(self,string):
        if string in self.d:
            return (self.d[string][2])
        else:return(r"Z20.0")
    def tval(self,string):
        if string in self.d:
            return (self.d[string][0])
        else:return(r"T2020")
    def report(self,string):
        if string in self.d:
            return (self.d[string])
        else:return(r"(no value)")


class GTable(GeometryTable):
    """for replacing geometries that were given
       default values before this program was created"""
    def load(self,location):
        GeometryTable.load(self,location)
        b=self.__dict__["d"].values()
        redict={x[0]:x for x in b}
        self.d=redict

##output dictionaries for regular expressions
##>>> test.d
##{'T0101': ['T0101', 'X22.960', 'Z20.0'],
## 'T0202': ['T0606', 'X32.425', 'Z7.376'],
## 'T0303': ['T0404', 'X30.588', 'Z15.620'],
## 'T0404': ['T0202', 'X22.367', 'Z18.549'],
## 'T0505': ['T0808', 'X33.291', 'Z8.197']}
share|improve this question

2 Answers 2

up vote 3 down vote accepted

Misnomer

GeometryTable.create() is poorly named: it doesn't create a GeometryTable. Rather, it loads tabular geometry data from a file. Therefore, it should be called load() instead. With that simple renaming, the following code starts to make sense:

table = GeometryTable()
table.load('part1.dat')    # Loads some data
table.load('part2.dat')    # Loads more data into the same object

Inappropriate inheritance, Part I

GeometryTable inherits from dict. Why? Would you say that GeometryTable is a dict, or that GeometryTable uses a dict? The latter is clearly true: it uses a dict called self.d. The former isn't true: you couldn't meaningfully write code like

table = GeometryTable()
table.load('something.dat')
table['T0202']             # ← Produces a KeyError

Therefore, as you've designed it, GeometryTable should just be an object, not a dict.

That said, you could change GeometryTable so that it is a dict, such that you can call table['T0202'].x. I don't really recommend doing so. "Prefer composition over inheritance" is a general maxim of object-oriented design.

Meaningless array indices

Expressions like tablelist[count+2] and self.d[string][1] are mysterious and hard to understand. I recommend the use of namedtuple so that you can have meaningful names like self.d[string].x instead.

Too much code

You're working too hard. load() could be written in five lines. Furthermore, for readability, efficiency, and scalability, it's better to work line by line rather than reading the entire file at once.

from collections import defaultdict, namedtuple

class GeometryTable(object):
    Row = namedtuple('Row', ['t', 'x', 'z'])
    default_row = lambda cls: cls.Row('T2020', 'X20.0', 'Z20.0')

    def __init__(self):
        self.d = defaultdict(self.default_row)

    def load(self, filename):
        """
        Loads data in the following format from the named file:

            |HAAS T|TARGET T|XGEOM|ZGEOM
            $T0101$T0101$X22.960$Z20.0$
            $T0202$T0606$X32.425$Z7.376$
            $T0303$T0404$X30.588$Z15.620$
            $T0404$T0202$X22.367$Z18.549$
            $T0505$T0808$X33.291$Z8.197$
        """
        with open(filename) as f:
            next(f)     # Discard first line, which contains a header
            for line in f:
                cols = line.split('$')
                self.d[cols[1]] = self.Row(*cols[2:5])

    def tval(self, haas):
        return self.d[haas].t

    def xval(self, haas):
        return self.d[haas].x

    def zval(self, haas):
        return self.d[haas].z

    def report(self, haas):
        return self.d.get(haas, '(no value)')

Alternatively, use the csv module:

import csv

class GeometryTable(object):
    …
    def load(self, filename):
        with open(filename) as f:
            next(f)     # Discard first line, which contains a header
            rows = csv.reader(f, delimiter='$')
            for cols in rows:
                self.d[cols[1]] = self.Row(*cols[2:5])

One minor deviation from your original code is that .report() returns a namedtuple rather than a list. However, considering that it sometimes returns a string instead (which is weird), I assume that that is an inconsequential detail.

Inappropriate inheritance, Part II

Your GTable class doesn't actually work. For one thing, Geometrytable is mis-capitalized, so it's evidently untested code.

The call to Geometrytable.create(self,location) would be better written as super().load(filename) (with the renaming suggested earlier).

Instead of self.__dict__["d"], why not just write self.d?

The biggest problem, though, is that a GTable simply isn't a GeometryTable. The methods .tval(), .xval(), and .zval() will all give wrong results, since their implementation is simply incompatible GTable's data representation. I have no idea what you are trying to accomplish with GTable, but inheritance is not the right solution.

share|improve this answer
    
Namedtuple feature as well as the use of defaults. This was very informative and helpful, thank you. –  user3567104 Apr 24 at 19:38

So there are a few improvements that I can see in your code, mainly in the create function.


When reading a file, use the with keyword. This will open the file and, once the program leaves that scope, the file will automatically be closed.

I would recommend changing how you read the file. From what I can tell, each line will be its own key in your dict. So instead of reading the entire file in one statement, I would recommend reading line-by-line.

By making the changes mentioned above, when we split the input by $, we get unnecessary values in the first and last index of the resulting list. To fix this, we can use list slicing. By using the notation list[start:end], where start specifies the first index you want included and stop specifies the first index you want excluded, we can remove the first and last indices.

Finally, when creating a dict you can assign a key-value pair simply by using the syntax:

dict[key] = value

Bringing all of these suggestions together, here is my version of your create function:

def create(self, location):
    # Once this block of code finishes, f will close.
    with open(location, 'r') as f:
        # Reads the file line-by-line
        for line in f:
            # Split the data and remove the first and last indices
            data = line.split('$')[1:-1]

            # Assign the key-value pair to the dict
            self.d[data[0]] = data[1:]

As a final aside, look over PEP8. PEP8 is the official Python style guide. Following those conventions (especially underscores in variable names, so table_string instead of tablestring) will help your code look and feel more Pythonic.

share|improve this answer
    
That worked really well with one small tweak. Added: if len(data)>1:self.d[data[0]] = data[1:] to handle index out of range error. Thankyou –  user3567104 Apr 24 at 6:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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