2
\$\begingroup\$

I'm making a code that, starting from an xml file:

  • stores the index of child elements of a tag and the child elements as key, values in a dictionary;
  • deletes keys whose values contain a certain string;
  • joins the dict values and extracts their text;
  • replaces certain values with "";
  • counts the occurrences of specific regex I specify.

Everything works fine, I just don't really know how to put these steps into working functions, as I tried and I can't make the return values taken from other functions. How to make my code more neat?

Here is my code that works but is not neat at all:

from xml.dom import minidom
import re

project_path = "output2.xml"
item_group_tag = "new_line"
cl_compile_tag = "text"

mydict = {}

def main():
    tree = minidom.parse(project_path)
    item_group_nodes = tree.getElementsByTagName(item_group_tag)
    for idx, item_group_node in enumerate(item_group_nodes):
        #print("{} {} ------------------".format(item_group_tag, idx))
        cl_compile_nodes = item_group_node.getElementsByTagName(cl_compile_tag)
        for cl_compile_node in cl_compile_nodes:
            #print("\t{}".format(cl_compile_node.toxml()))
            mydict[idx]=[item_group_node.toxml()]


if __name__ == "__main__":
    main()

listakey = []

def search(values, searchFor):
    for k in values:
        for v in values[k]:
            if searchFor in v:
                #return k
                print(k)
                listakey.append(k)
    return None

#Checking if string 'Mary' exists in dictionary value
print(search(mydict, '10.238')) #prints firstName
#print(listakey)

for x in listakey:
    del mydict[x]

mylist = []
uncinata1 = " < "
uncinata2 = " >"
punto = "."
virgola = ","
puntoevirgola = ";"
dash ="-"
puntoesclamativo ="!"
duepunti = ":"
apostrofo ="’"
puntointerrogativo = "?"
angolate ="<>"


#print(mydict.values())
for value in mydict.values():
    myxml = ' '.join(value)
    #print(myxml)
    import xml.etree.ElementTree as ET

    tree = ET.fromstring(myxml)
    lista = ([text.text for text in tree.findall('text')])
    testo = (' '.join(lista))
    testo = testo.replace(uncinata1, "")
    testo = testo.replace(uncinata2, "")
    testo = testo.replace(punto, "")
    testo = testo.replace(virgola, "")
    testo = testo.replace(puntoevirgola, "")
    testo = testo.replace(dash, "")
    testo = testo.replace(puntoesclamativo, "")
    testo = testo.replace(duepunti, "")
    testo = testo.replace(apostrofo, "")
    testo = testo.replace(puntointerrogativo, "")
    testo = testo.replace(angolate, "")
    print(testo)

    find_prima = re.compile(r"\]\s*prima(?!\S)")
    find_fase_base = re.compile(r"\]\s*AN\s*([\w\s]+)\s*da\scui\sT")# ] AN parole da cui T
    find_fase_base_2 = re.compile(r"\]\s([\w\s]+)\s[→]\sT") #] parole → T
    find_fase_base_3 = re.compile(r"\]\s*([\w\s]+)\s*da\scui\sT")  # ] parole da cui T
    find_fase_12 = re.compile(r"\]\s1\s([\w\s]+)\s2\s([\w\s]+[^T])") #] 1 parole 2 parole (esclude T)
    find_fase_12_leo = re.compile(r"(?!.*da cui)\]\s+AN\s1\s+([a-zA-Z]+(?:\s+[a-zA-Z]+)*)\s+2\s+([a-zA-Z]+(?:\s+[a-zA-Z]+)*)")#] AN 1 parole da cui 2 parole escludendo da cui dopo
    find_fase_12T_leo = re.compile(r"\]\s*AN\s*1\s*([\w\s]+)da\s*cui\s*2\s*([\w\s]+)da\s*cui\s*T") # ] AN 1 parole da cui 2 parole parola da cui T
    matches_prima = re.findall(find_prima, testo)
    matches_fb2 = re.findall(find_fase_12, testo)
    lunghezza = len(matches_fb2)
    mylist.append(lunghezza)


count = 0
for elem in mylist:
    count += elem

print(count)

The final goal would be to create a count function for each regex I specify.

EDIT: sample XML file:

<pages>
  <page id="1" bbox="0.000,0.000,462.047,680.315" rotate="0">
    <textbox id="0" bbox="191.745,592.218,249.042,603.578">
<textline>
     <new_line>
              <text font="NUMPTY+ImprintMTnum" bbox="297.284,540.828,300.188,553.310" colourspace="DeviceGray" ncolour="0" size="12.482">della quale non conosce che una parte;] </text>
              <text font="PYNIYO+ImprintMTnum-Italic" bbox="322.455,540.839,328.251,553.566" colourspace="DeviceGray" ncolour="0" size="12.727">prima</text>
              <text font="NUMPTY+ImprintMTnum" bbox="331.206,545.345,334.683,552.834" colourspace="DeviceGray" ncolour="0" size="7.489">1</text>
              <text font="NUMPTY+ImprintMTnum" bbox="177.602,528.028,180.850,540.510" colourspace="DeviceGray" ncolour="0" size="12.482">che nonconosce ancora appieno;</text>
              <text font="NUMPTY+ImprintMTnum" bbox="189.430,532.545,192.908,540.034" colourspace="DeviceGray" ncolour="0" size="7.489">2</text>
              <text font="NUMPTY+ImprintMTnum" bbox="203.879,528.028,208.975,540.510" colourspace="DeviceGray" ncolour="0" size="12.482">che</text>
            </new_line>
</textline>
</textbox>
</page>
</pages>

I want just to return the count of the regex I specified.

\$\endgroup\$
3
  • \$\begingroup\$ Welcome to CR. Can you attach a sample xml file and show expected results ? \$\endgroup\$ – Anonymous Apr 20 '20 at 13:49
  • \$\begingroup\$ Done, see update, let me know if it helps \$\endgroup\$ – Anna Apr 20 '20 at 13:54
  • 2
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 20 '20 at 17:45
3
\$\begingroup\$

First of all, welcome to CR and Python world. Let's start from the beginning.

Imports

In Python, the imports are usually put at the top:

import re
from xml.dom import minidom
from xml.etree import ElementTree as ET

Functions

You created some functions which you might want to group together:

def search(values, searchFor):
    for k in values:
        for v in values[k]:
            if searchFor in v:
                #return k
                print(k)
                listakey.append(k)
    return None


def main():
    tree = minidom.parse(project_path)
    item_group_nodes = tree.getElementsByTagName(item_group_tag)
    for idx, item_group_node in enumerate(item_group_nodes):
        #print("{} {} ------------------".format(item_group_tag, idx))
        cl_compile_nodes = item_group_node.getElementsByTagName(cl_compile_tag)
        for cl_compile_node in cl_compile_nodes:
            #print("\t{}".format(cl_compile_node.toxml()))
            mydict[idx]=[item_group_node.toxml()]

Let's first improve these functions.

search

In this function it looks like you're creating a new list which contains items that contain a specific value in valuess ... values. The naming of your variables and function are both confusing and don't stick to the recommended styling conventions.

def filter_values_by_keyword(my_dict, filter_by):
    """
    Return a list of values which contains `filter_by` keyword.
    
    Arguments:
        my_dict (dict): Dict containing (...data specifics here)
        filter_by (str): Keyword to look for in values of my_dict
        
    Return:
        List of filtered values
    """
    return [value for key, value in my_dict.items() if filter_by in value]

Now this is how I would reimplement your search function. As you can see, you can now figure out what the function is doing only by looking at its name and parameters. If that's not enough, I've added a docstring to better describe what the function does.

main

Now, this doesn't look like a proper main function. The main() function of a program usually contains all the logic within a program ... which is not happening here. It looks like you're just parsing a xml file and add some specific data to a dictionary. Let's rename our function and add some improvements to it.

def get_xml_by_tag_names(xml_path, tag_name_1, tag_name_2):
    data = {}
    xml_tree = minidom.parse(xml_path)
    item_group_nodes = xml_tree.getElementsByTagName(tag_name_1)
    for idx, item_group_node in enumerate(item_group_nodes):
        cl_compile_nodes = item_group_node.getElementsByTagName(tag_name_2)
        for _ in cl_compile_nodes:
            data[idx]=[item_group_node.toxml()]
    return data

Now it's a bit better. We're generating a dictionary from an XML by looking after specific tag names. You can add docstrings to this to make the functionality even more clearer.

Now that you've moved the non-main logic into a proper function, let's add all the remaining lines into a proper main function:

def main():
    data = get_xml_by_tag_names('output2.xml', 'new_line', 'text')
    filtered_values = filter_values_by_keyword(data, '10.238')
    for item in filtered_values:
        del data[item]

    mylist = []
    uncinata1 = " < "
    uncinata2 = " >"
    punto = "."
    virgola = ","
    puntoevirgola = ";"
    dash = "-"
    puntoesclamativo = "!"
    duepunti = ":"
    apostrofo = "’"
    puntointerrogativo = "?"
    angolate = "<>"

    for value in data.values():
        myxml = ' '.join(value)
        # print(myxml)

        tree = ET.fromstring(myxml)
        lista = ([text.text for text in tree.findall('text')])
        testo = (' '.join(lista))
        testo = testo.replace(uncinata1, "")
        testo = testo.replace(uncinata2, "")
        testo = testo.replace(punto, "")
        testo = testo.replace(virgola, "")
        testo = testo.replace(puntoevirgola, "")
        testo = testo.replace(dash, "")
        testo = testo.replace(puntoesclamativo, "")
        testo = testo.replace(duepunti, "")
        testo = testo.replace(apostrofo, "")
        testo = testo.replace(puntointerrogativo, "")
        testo = testo.replace(angolate, "")
        print(testo)

        find_prima = re.compile(r"\]\s*prima(?!\S)")
        find_fase_base = re.compile(r"\]\s*AN\s*([\w\s]+)\s*da\scui\sT")  # ] AN parole da cui T
        find_fase_base_2 = re.compile(r"\]\s([\w\s]+)\s[→]\sT")  # ] parole → T
        find_fase_base_3 = re.compile(r"\]\s*([\w\s]+)\s*da\scui\sT")  # ] parole da cui T
        find_fase_12 = re.compile(r"\]\s1\s([\w\s]+)\s2\s([\w\s]+[^T])")  # ] 1 parole 2 parole (esclude T)
        find_fase_12_leo = re.compile(
            r"(?!.*da cui)\]\s+AN\s1\s+([a-zA-Z]+(?:\s+[a-zA-Z]+)*)\s+2\s+([a-zA-Z]+(?:\s+[a-zA-Z]+)*)")  # ] AN 1 parole da cui 2 parole escludendo da cui dopo
        find_fase_12T_leo = re.compile(
            r"\]\s*AN\s*1\s*([\w\s]+)da\s*cui\s*2\s*([\w\s]+)da\s*cui\s*T")  # ] AN 1 parole da cui 2 parole parola da cui T
        matches_prima = re.findall(find_prima, testo)
        matches_fb2 = re.findall(find_fase_12, testo)
        lunghezza = len(matches_fb2)
        mylist.append(lunghezza)

    count = 0
    for elem in mylist:
        count += elem

    print(count)

This main() function can be also refactored quite a bit but unfortunately I don't have enough time at the moment. Here's the full code for my proposed changes:

import re
from xml.dom import minidom
from xml.etree import ElementTree as ET


def filter_values_by_keyword(my_dict, filter_by):
    """
    Return a list of values which contains `filter_by` keyword.

    Arguments:
        my_dict (dict): Dict containing (...data specifics here)
        filter_by (str): Keyword to look for in values of my_dict

    Return:
        List of filtered values
    """
    return [value for key, value in my_dict.items() if filter_by in value]


def get_xml_by_tag_names(xml_path, tag_name_1, tag_name_2):
    """
    Your docstring here.
    """
    data = {}
    xml_tree = minidom.parse(xml_path)
    item_group_nodes = xml_tree.getElementsByTagName(tag_name_1)
    for idx, item_group_node in enumerate(item_group_nodes):
        cl_compile_nodes = item_group_node.getElementsByTagName(tag_name_2)
        for _ in cl_compile_nodes:
            data[idx]=[item_group_node.toxml()]
    return data


def main():
    data = get_xml_by_tag_names('output2.xml', 'new_line', 'text')
    
    filtered_values = filter_values_by_keyword(data, '10.238')
    for item in filtered_values:
        del data[item]

    mylist = []
    uncinata1 = " < "
    uncinata2 = " >"
    punto = "."
    virgola = ","
    puntoevirgola = ";"
    dash = "-"
    puntoesclamativo = "!"
    duepunti = ":"
    apostrofo = "’"
    puntointerrogativo = "?"
    angolate = "<>"

    for value in data.values():
        myxml = ' '.join(value)
        # print(myxml)

        tree = ET.fromstring(myxml)
        lista = ([text.text for text in tree.findall('text')])
        testo = (' '.join(lista))
        testo = testo.replace(uncinata1, "")
        testo = testo.replace(uncinata2, "")
        testo = testo.replace(punto, "")
        testo = testo.replace(virgola, "")
        testo = testo.replace(puntoevirgola, "")
        testo = testo.replace(dash, "")
        testo = testo.replace(puntoesclamativo, "")
        testo = testo.replace(duepunti, "")
        testo = testo.replace(apostrofo, "")
        testo = testo.replace(puntointerrogativo, "")
        testo = testo.replace(angolate, "")
        print(testo)

        find_prima = re.compile(r"\]\s*prima(?!\S)")
        find_fase_base = re.compile(r"\]\s*AN\s*([\w\s]+)\s*da\scui\sT")  # ] AN parole da cui T
        find_fase_base_2 = re.compile(r"\]\s([\w\s]+)\s[→]\sT")  # ] parole → T
        find_fase_base_3 = re.compile(r"\]\s*([\w\s]+)\s*da\scui\sT")  # ] parole da cui T
        find_fase_12 = re.compile(r"\]\s1\s([\w\s]+)\s2\s([\w\s]+[^T])")  # ] 1 parole 2 parole (esclude T)
        find_fase_12_leo = re.compile(
            r"(?!.*da cui)\]\s+AN\s1\s+([a-zA-Z]+(?:\s+[a-zA-Z]+)*)\s+2\s+([a-zA-Z]+(?:\s+[a-zA-Z]+)*)")  # ] AN 1 parole da cui 2 parole escludendo da cui dopo
        find_fase_12T_leo = re.compile(
            r"\]\s*AN\s*1\s*([\w\s]+)da\s*cui\s*2\s*([\w\s]+)da\s*cui\s*T")  # ] AN 1 parole da cui 2 parole parola da cui T
        matches_prima = re.findall(find_prima, testo)
        matches_fb2 = re.findall(find_fase_12, testo)
        lunghezza = len(matches_fb2)
        mylist.append(lunghezza)

    count = 0
    for elem in mylist:
        count += elem

    print(count)

if __name__ == "__main__":
    main()

```
\$\endgroup\$
2
  • \$\begingroup\$ Thank you, it just now doesn't work the deletion of the item if the value of the dict contains "10.238"... \$\endgroup\$ – Anna Apr 20 '20 at 17:30
  • \$\begingroup\$ I want to delete the whole dict entry if there is that value, I mean \$\endgroup\$ – Anna Apr 20 '20 at 17:47

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.