1
\$\begingroup\$

so this is my code and I just want to improve it, I'm a beginner so I think there are some better and short ways to do what I did here. Any Ideas? The code:

class Table:

    def __init__(self,path,sep):
        self.path=path
        self.sep=sep
        self.g=[]
        self.count=0
        self.headlines=[]
        self.matrix=[]
        self.headrows=[]
        self.postionrow=0
        self.postioncolmn=0
        try:
            f=open(self.path,'r')

            read_file=f.read()
            split_file=read_file.split()
            for line in split_file:
                list_the_line=line.split(self.sep)
                self.g.append(list_the_line)
            self.count=0    
            for z in range (len(self.g[0])):
                self.count=0 
                for d in range(len(self.g[0])):
                    if self.g[0][z]==self.g[0][d]:
                        self.count+=1
                if self.count>=2:
                    raise ValueError        




            num_first_line=len(self.g[0])
            for k in range (len(self.g)):
                if len(self.g[k])!= num_first_line:
                    raise ValueError

            self.headlines=self.g[0]        
            self.g.remove(self.g[0])

            self.count=0
            for row_name1 in range (len(self.g)):
                self.count=0
                for row_name2 in range(len(self.g)):
                    if self.g[row_name1][0]==self.g[row_name2][0]:
                        self.count+=1

                if self.count>=2:
                    raise ValueError







            for i in range (len(self.g)):
                self.headrows.append(self.g[i][0])
                self.g[i].remove(self.g[i][0])

            ezer=[]

            for op in range (len(self.g)):
                ezer=[]
                for od in range (len(self.g[0])):
                    ezer.append(self.g[od][op])
                self.matrix.append(ezer)    


            f.close()    




        except :
            print "Check - Error..."
            return

    def len(self):
        num_rows=len(self.g)
        num_cols=len(self.g[0])
        return num_rows*num_cols

    def get_row(self,rowname):
        for i in range (len(self.headlines)):
            if rowname==self.headrows[i]:
                self.postionrow=i
                return self.g[i]

        if not rowname in self.headrows :
            raise ValueError
    def get_column(self,colname):
        for i in range (len(self.headlines)):
            if colname==self.headlines[i]:
                self.postioncolmn=i-1

                return self.matrix[i-1]

        if not colname in self.headlines :
            raise ValueError
    def get_value(self,rowname,colname):
        self.get_row(rowname)
        self.get_column(colname)
        if not rowname in self.headrows :
            raise ValueError
        if not colname in self.headlines :
            raise ValueError

        return self.g[self.postionrow][self.postioncolmn]

    def get_row_name_with_max_value(self,colname):
        if not colname in self.headlines :
            raise ValueError
        max_colmn=max(self.get_column(colname))
        for i in range (len(self.matrix)):

            if max_colmn == self.g[i][self.postioncolmn]:

                return self.headrows[i]

and what should be the result:

>>> table = Table("table_examp1111111","\t")
Check - Error...
>>> table = Table("table_example1.txt","\t")             
>>> print table.len() 
12 
>>> print table.get_row("Menny")
['M', '1', '1', '1'] 
>>> print table.get_column("Height") 
['1', '2', '3'] 
>>> print table.get_value("Sami","Age")
3
>>> print table.get_row_name_with_max_value("Height")
Sami
>>> print table.get_row_name_with_max_value("Salary")
Sami

This code works but I want to make it more Pythonic. Please don't change the form, don't add or remove function just fix my syntax and make it better.

\$\endgroup\$
2
  • \$\begingroup\$ Did you consider using the built-in sqlite3 module instead? \$\endgroup\$ Dec 10, 2013 at 18:56
  • \$\begingroup\$ Thank you for your idead, but sorry, I don't want to use this build-in function... just using the basic built in function like "sum" or "len"... \$\endgroup\$
    – Bmap
    Dec 10, 2013 at 18:59

1 Answer 1

3
\$\begingroup\$

Here are a number of suggestions that would help the readability and usability of your code. Most of these are fairly high level suggestions that apply almost anywhere, but some do get into the details of your implementation.

  • Comment on the blocks that make up your Table.__init__, or even better, refactor them into helpers. For example the first six lines in the try initialize self.g, but the next several are verifying some unclear invariant (it seems to be checking for duplicates), and the next couple appear to verify that each line had the same number of items. These blocks would be much more clear with either a method name or comment describing what it was verifying.
  • Avoid for i in range(len(mylist)): ... mylist[i] ... loops unless performance measurements have shown it to be necessary. Instead prefer loops starting with for item in mylist: or (if you need the index) for i, item in enumerate(mylist): ...
  • Don't use mylist.remove(mylist[0]) where del mylist[0] or perhaps a slight rewrite and mylist.pop(0) would work better. The call to remove is more indirect, and thus requires more thought to assess its impact. For example, applying both of these points:

     # original:
     for i in range (len(self.g)):
         self.headrows.append(self.g[i][0])
         self.g[i].remove(self.g[i][0])
    
     # better (if line is an appropriate name):
     for line in self.g:
         self.headrows.append(line.pop(0))
    
  • Avoid try: ... except: ... clauses that do not specify an exception type. These suppress exceptions like KeyboardInterrupt that you probably do not want to suppress. Also limit the length of the try block to as short as you can feasibly make it. This one covers at least two kinds of possible exceptions (a failure opening or reading the file, and a failure processing the file).

  • Avoid just suppressing an important exception like __init__ does; after all, exceptions are the only way for an __init__ to signal an error. When an exception occurs in the middle of processing the file, the code prints a message, and returns with a partially initialized object. This means calling code can't tell there was an error, and may make invalid assumptions about the contents of the file. If you really need the partially initialized object, you can look into returning it on a custom exception type (this need is fairly rare).
  • Looking past __init__, the various error cases across several methods all raise a ValueError when it looks like KeyError or IndexError would be more appropriate. You can get this for free if you slightly restructure your code to maintain a dict mapping names to column/row indices, and just let the code attempt to access things. If the key isn't in the mapping, the dict will raise the KeyError. You can also avoid your lookup loops. Win-win.
\$\endgroup\$

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.