7
\$\begingroup\$

I have written a script that turns an asn file into a hpp file, but it look very dirty even though I have made changes based on pylint.

I think I make use of too many lists, for-loops and try except blocks.

I would like to get a review of my code, and I am particularly interested in making my code look more organized and make my code more efficient.

Sample input

SerError ::= CHO
{
   app-ref [0] IMPLICIT ENUMERATED
   {
      --  provider only
      other (0),
      ti-ela (1)


   },
   ha-res [1] IMPLICIT ENUMERATED
   {
      --  hardware 
      other (0),
      me-unavailable (1)

   }
}

Sample output

enum ESererror//SSerError ::= CHO
{
   eSererrorAppRef = 0, 
   eSererrorHaRes = 1
};

enum EAppRef//pp-ref [0] IMPLICIT ENUMERATED
{
   //--  provider only
   eAppRefOther = 0,
   eAppRefTiEla = 1
};

enum EHaRes//ha-res [1] IMPLICIT ENUMERATED
{
   //-- VDE hardware troubles
   eHaResOther = 0,
   eHaResMeUnavailable = 1
};

This is the script I would like to have reviewed:

'''
************************************************************************

@brief 
@version 1.0

************************************************************************
'''
import re
from collections import deque
import sys
import inflection


class Convert(object):
    '''To do: add data'''
    def __init__(self):
        '''To do: add data'''
        self.plist = []
        self.slist = []
        self.tlist = []
        self.llist = []
        self.lines = []
        self.line = None
        self.open_braces = []
        self.close_braces = []
        self.outputfile = None
        self.i = None
        self.open_brace_list = []
        self.close_brace_list = []
        self.file_name = None
        self.split_character = None
        self.length = None
        self.enumvariable_flag = None
        self.inner_variable_prefix=""

    def start_tag(self, split_character, line):
        '''To do: add data'''
        self.split_character = split_character
        self.line = line
        line_to_write = ""
        line_to_write += "enum E"
        self.inner_variable_prefix = (inflection.camelize(((self.line.split(self.split_character)[0]).replace('-', '_')).lower())).strip()
        line_to_write += self.inner_variable_prefix
        line_to_write += "//"+self.line
        line_to_write += "\n"
        line_to_write += "{\n"
        self.outputfile.write(line_to_write)
        self.enumvariable_flag = True

    def end_tag(self,line):
        self.line=line
        line_to_write = ""
        line_to_write += "};\n"
        self.outputfile.write(line_to_write)
        self.enumvariable_flag = False

    def comment_line(self, line):
        self.line = line
        line_to_write = ""
        line_to_write += "//"+self.line+"\n"
        self.outputfile.write(line_to_write)
    def handle_comment(self, line):
        '''To do: add data'''
        self.line = line
        if (line.strip()).startswith("--")or(re.search(r'(.*)\{(.*)\}(.*)', line)):
            line_to_write = "   "
            line_to_write += "//"+self.line+"\n"
            self.outputfile.write(line_to_write)

    def handle_inner_element(self, line, index):
        '''To do: add data'''
        self.line = line
        self.index = index
        line_to_write = "   "
        try:
            try:
                value = (re.findall(r'\d+', self.line.strip().split(' ')[1])[0])
                line_to_write += "e"
                line_to_write += self.inner_variable_prefix
                line_to_write += inflection.camelize((self.line.strip().split(' ')[0]).replace('-', '_'))
                line_to_write += " = "
                line_to_write += value
                if self.index not in self.llist:
                    line_to_write += ","
                line_to_write += "\n"
            except:
                if (self.line.strip().split(' ')[0]).lower() == \
                   self.line.strip().split(' ')[1].split('-')[0].lower():
                    line_to_write += "e"
                    line_to_write += self.inner_variable_prefix
                    line_to_write += inflection.camelize((
                        self.line.strip().split(' ')[0].replace('-', '_')).lower())
                    if self.index not in self.llist:
                        line_to_write += ","
                else:
                    line_to_write += "//"
                    line_to_write += self.line
                line_to_write += "\n"
        except Exception as exception:
            print(exception)
        self.outputfile.write(line_to_write)

    def generate_lists(self, length, lines):
        '''To do: add data'''
        self.length = length
        self.lines = lines
        flag_llist=False
        lastl=None
        for index, line in enumerate(self.lines):
            reg1 = r'::=(.*)\n\{'

            reg2 = r'{'

            reg3 = r'\}'

            reg4 = r'(.*)\{(.*)\}(.*)'
            if index < (self.length-1):
                val = str(line) + "\n" + str(self.lines[index+1])
            else:
                val = str(line)
            if re.search(reg1, val)and(not re.search(reg4, val)):
                self.plist.append(index)
                flag_llist=True
            else:
                val = str(line)
            if re.search(reg2, val)and(not re.search(reg4, val)):
                if index in self.plist:
                    pass
                else:
                    self.slist.append(index)
                    flag_llist=True
            if re.search(reg3, val)and(not re.search(reg4, val)):
                self.tlist.append(index)
                self.llist.append(lastl)
                flag_llist=False
            elif flag_llist:
                try:
                    value = (re.findall(r'\d+', line.strip().split(' ')[1])[0])
                    lastl=index
                except:
                    pass
                try:
                    if (line.strip().split(' ')[0]).lower() == \
                       line.strip().split(' ')[1].split('-')[0].lower():
                        lastl=index
                except:
                    pass

        return self.plist, self.slist, self.tlist

    def add_sub_element(self, open_brace_list, close_brace_list):
        '''To do: add data'''
        self.open_brace_list = open_brace_list
        self.close_brace_list = close_brace_list
        line_to_write = ""
        self.enumvariable_flag = False
        for i in range(1, len(self.open_brace_list)):
            for index, line in enumerate(self.lines):
                if index == self.open_brace_list[i]-1:
                    self.start_tag(' ', line)
                if (index <= self.close_brace_list[i-1])and\
                   (index > self.open_brace_list[i])and self.enumvariable_flag:
                    self.handle_comment(line)
                    if (self.line.strip()).startswith("}"):
                        self.end_tag(line)
                if self.enumvariable_flag and(not (self.line.strip()).startswith("--"))and\
                   (not (self.line.strip()).startswith("{")and\
                    (index <= self.close_brace_list[i-1])and(index > open_brace_list[i])):
                    self.handle_inner_element(line, index)

    def braces_line_no(self, i):
        '''To do: add data'''
        self.i = i
        remaining_slist = [a for a in self.slist if a > self.plist[self.i]]
        remaining_tlist = [a for a in self.tlist if a > self.plist[self.i]]
        try:
            self.open_braces = [b for b in remaining_slist if b < self.plist[self.i+1]]
        except:
            self.open_braces = remaining_slist
        try:
            self.close_braces = [b for b in remaining_tlist if b < self.plist[self.i+1]]
        except:
            self.close_braces = remaining_tlist
        return self.open_braces, self.close_braces

    def generate_output(self, file_name):
        '''To do: add data'''
        self.file_name = file_name
        output_file_name = self.file_name.split('.')[0]+".hpp"
        self.outputfile = open(output_file_name, 'w')
        with open(self.file_name) as f_in:
            self.lines = (line.strip() for line in f_in)
            self.lines = list(line for line in self.lines if line)
        length = len(self.lines)
        self.plist, self.slist, self.tlist = self.generate_lists(length, self.lines)
        for i in range(len(self.plist)):
            line_to_write = ""
            self.open_braces, self.close_braces = self.braces_line_no(i)
            open_braces_qu = deque(self.open_braces)
            for index, line in enumerate(self.lines):
                if (not self.enumvariable_flag)and(self.tlist[-1] != self.close_braces[-1]):
                    if(index > self.close_braces[-1]) and (index < self.slist[self.slist.index(self.open_braces[-1])+1]-1):
                            self.comment_line(line)
                    elif self.enumvariable_flag==None and (index < self.plist[0]):
                        self.comment_line(line)
                elif self.close_braces[-1] == self.tlist[-1] and index > self.tlist[-1]:
                    self.comment_line(line)
                if index == self.plist[i]:
                    self.start_tag('::=', line)
                elif len(self.open_braces) == 1 and len(self.close_braces) == 1 and\
                     self.enumvariable_flag:
                    self.handle_comment(line)
                    if (self.line.strip()).startswith("}"):
                        self.end_tag(line)
                    if self.enumvariable_flag and(not (line.strip()).startswith("--"))and\
                       (not (line.strip()).startswith("{")):
                        self.handle_inner_element(line, index)

                elif self.enumvariable_flag and(len(self.open_braces) > 1)and(len(open_braces_qu) > 1):
                    line_to_write = "   "
                    try:
                        if index == open_braces_qu[1]-1:
                            try:
                                value = (re.findall(r'\d+', line.strip().split(' ')[1])[0])
                                line_to_write += "e"
                                line_to_write += self.inner_variable_prefix
                                line_to_write += inflection.camelize((line.strip().split(' ')[0]\
                                                                      .replace('-', '_')).lower())
                                line_to_write += " = "
                                line_to_write += value
                                if len(open_braces_qu) > 2:
                                    line_to_write += ", "
                                line_to_write += "\n"
                            except:
                                if (line.strip().split(' ')[0]).lower() == line.strip()\
                                   .split(' ')[1].split('-')[0].lower():
                                    line_to_write += "e"
                                    line_to_write += self.inner_variable_prefix
                                    line_to_write += inflection.camelize((line.strip()\
                                    .split(' ')[0].replace('-', '_')).lower())
                                    if len(open_braces_qu) > 2:
                                        line_to_write += ", "
                                else:
                                    line_to_write += "//"
                                    line_to_write += line
                                line_to_write += "\n"
                            self.outputfile.write(line_to_write)
                            open_braces_qu.popleft()
                            if  len(open_braces_qu) == 1:
                                self.end_tag(line)
                                open_braces_qu.popleft()
                                self.add_sub_element(self.open_braces, self.close_braces)

                    except Exception as exception:
                        print(exception)

        self.outputfile.close()

if __name__ == '__main__':
    INPUT_FILE_NAME = sys.argv[1]
    CON_OBJ = Convert()
    CON_OBJ.generate_output(INPUT_FILE_NAME)
\$\endgroup\$
  • 2
    \$\begingroup\$ It would help if you could summarize what the script is for and change the title to be more descriptive. \$\endgroup\$ – Graipher Aug 24 '16 at 7:52
5
\$\begingroup\$

You should also never have a bare except. Always use at least except Exception. Because otherwise you can't e.g. abort with CTRL-C anymore if it is stuck.

You should also try to limit their scope as small as possible to guard only the one operation which may fail. Otherwise it becomes a lot harder to debug as well.

\$\endgroup\$
3
\$\begingroup\$
  • The outputfile should be protected with with open(...) as ... as well, just assign it to self.outputfile in that block.
  • reg1 to reg3 are only used once, perhaps just inline them.
  • brace_lines has duplicated code, I'd try to split it so that you can just pass in the two lists to a shared method/function.
\$\endgroup\$
3
\$\begingroup\$

In Python, strings are not mutable. What this means is that every time you do line_to_write += "foo_barred" a new string will be created which is a concatenated version of the two strings, and the new string will then be stored into line_to_print.

This may not seem very bad, but it can sneak up on you in a couple different ways:

  1. Short strings being concatenated in most cases can be faster. However, using concatenation on large strings and in a loop will result in poor performance.
  2. Strings being immutable necessarily means that when doing large amounts of manipulations on strings (esp short ones). Large amounts of new strings are being created, returned, maybe even cleaned up depending on how the interpreter is feeling today.

Regardless of whether the memory is being cleared properly however, this is likely to create large amounts of memory fragmentation due to constant alloc / dealloc / realloc... The smaller the strings and the faster you run, the more memory will get fragmented.

Ideally you should be doing less nesting of control flow. I generally try to keep anything deeper than ~3 control flow structures in a separate method since it will be easier to understand and maintain that way.

\$\endgroup\$
  • \$\begingroup\$ should I use data type such as list instead of string concatenation line_to_write to avoid fragmentation \$\endgroup\$ – Ravi Yadav Aug 25 '16 at 6:23
  • \$\begingroup\$ i have update my script with list...but the execution time has increased multi folds. so it that okay ? old script took 0.019999980926513672 seconds. >>> ================================ RESTART ================================ >>> new script took 0.2999999523162842 seconds. \$\endgroup\$ – Ravi Yadav Aug 25 '16 at 8:24
  • \$\begingroup\$ only if you are working with the same long string alot will it become much of an issue, all optimization comes down to 3 steps, Test how fast it runs(and where the time is coming from), Make modifications to the slowest bits, Retest and revert any Performance regressions. \$\endgroup\$ – DJHenjin Aug 28 '16 at 5:16
0
\$\begingroup\$

if you want it to look "clean" you can try reducing the line count. Reduce if blocks to comprehensions:

if index < (self.length-1):
    val = str(line) + "\n" + str(self.lines[index+1])
else:
    val = str(line)

to:

val = str(line) + "\n" + str(self.lines[index+1]) if index < (self.length-1) else str(line) 

reduce += calls

    line_to_write += self.inner_variable_prefix
    line_to_write += "//"+self.line
    line_to_write += "\n"
    line_to_write += "{\n"

to:

    line_to_write += self.inner_variable_prefix+"//"+self.line+"\n"+"{\n"

use less self variables self.i, its never used globally, only in braces_line_no()

When things are shorter you might be able to notice some improvements you can make to your control structure, as well as decrease the amount of lines you have which will make it easier to catch mistakes and view all your code.

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