4

This question was edited. Please see the edit on the bottom first.

This question is going to be a bit long so I'm sorry in advance. Please consider two different types of data:

Data A:

{
  "files": [
    {
      "name": "abc",
      "valid": [
        "func4",
        "func1",
        "func3"
      ],
      "invalid": [
        "func2",
        "func8"
      ]
    }
  ]
}

Data B:

{
  "files": [
    {
      "methods": {
        "invalid": [
          "func2",
          "func8"
        ],
        "valid": [
          "func4",
          "func1",
          "func3"
        ]
      },
      "classes": [
        {
          "invalid": [
            "class1",
            "class2"
          ],
          "valid": [
            "class8",
            "class5"
          ],
          "name": "class1"
        }
      ],
      "name": "abc"
    }
  ]
}

I'm trying to merge each file (A files with A and B files with B). Previous question helped me figure out how to do it but I got stuck again. As I said in the previous question there is a rule for merging the files. I'll explain again: Consider two dictionaries A1 and A2. I want to merge invalid of A1 with A2 and valid of A1 with A2. The merge should be easy enough but the problem is that the data of invalid and valid dependents on each other. The rule of that dependency - if number x is valid in A1 and invalid in A2 then its valid in the merged report. The only way to be invalid is to be in the invalid list of both of A1 and A2 (Or invalid in one of them while not existing in the other). In order to merge the A files I wrote the following code:

def merge_A_files(self, src_report):
    for current_file in src_report["files"]:
        filename_index = next((index for (index, d) in enumerate(self.A_report["files"]) if d["name"] == current_file["name"]), None)
        if filename_index == None:
            new_block = {}
            new_block['valid'] = current_file['valid']
            new_block['invalid'] = current_file['invalid']
            new_block['name'] = current_file['name']
            self.A_report["files"].append(new_block)
        else:
            block_to_merge = self.A_report["files"][filename_index]
            merged_block = {'valid': [], 'invalid': []}
            merged_block['valid'] = list(set(block_to_merge['valid'] + current_file['valid']))
            merged_block['invalid'] = list({i for l in [block_to_merge['invalid'], current_file['invalid']]
                                              for i in l if i not in merged_block['valid']})
            merged_block['name'] = current_file['name']
            self.A_report["files"][filename_index] = merged_block
            
            

For merging B files I wrote:

def _merge_functional_files(self, src_report):
    for current_file in src_report["files"]:
        filename_index = next((index for (index, d) in enumerate(self.B_report["files"]) if d["name"] == current_file["name"]), None)
        if filename_index == None:
            new_block = {'methods': {}, 'classes': []}
            new_block['methods']['valid'] = current_file['methods']['valid']
            new_block['methods']['invalid'] = current_file['methods']['invalid']
            new_block['classes'] += [{'valid': c['valid'],  'invalid': c['invalid'], 'name': c['name'] } for c in current_file['classes']]
            new_block['name'] = current_file['name']
            self.B_report["files"].append(new_block)
        else:
            block_to_merge = self.B_report["files"][filename_index]
            merged_block = {'methods': {}, 'classes': []}
            for current_class in block_to_merge["classes"]:
                current_classname = current_class.get("name")
                class_index = next((index for (index, d) in enumerate(merged_block["classes"]) if d["name"] == current_classname), None)
                if class_index == None:
                    merged_block['classes'] += ([{'valid': c['valid'],  'invalid': c['invalid'], 'name': c['name'] } for c in current_file['classes']])
                else:
                    class_block_to_merge = merged_block["classes"][class_index]
                    class_merged_block = {'valid': [], 'invalid': []}
                    class_merged_block['valid'] = list(set(class_block_to_merge['valid'] + current_class['valid']))
                    class_merged_block['invalid'] = list({i for l in [class_block_to_merge['invalid'], current_class['invalid']]
                                                            for i in l if i not in class_merged_block['valid']})
                    class_merged_block['name'] = current_classname
                    merged_block["classes"][filename_index] = class_merged_block

            merged_block['methods']['valid'] = list(set(block_to_merge['methods']['valid'] + current_file['methods']['valid']))
            merged_block['methods']['invalid'] = list({i for l in [block_to_merge['methods']['invalid'], current_file['methods']['invalid']]
                                                         for i in l if i not in merged_block['methods']['valid']})
            merged_block['name'] = current_file['name']
            self.B_report["files"][filename_index] = merged_block

It looks like the code of A is valid and works as expected. But I have a problem with B, especially with merging classes. The example I have problem with:

First file:

{
  "files": [
    {
      "name": "some_file1",
      "methods": {
        "valid": [
          "func4",
          "func1"
        ],
        "invalid": [
          "func3"
        ]
      },
      "classes": [
        {
          "name": "class1",
          "valid": [
            "class1",
            "class2"
          ],
          "invalid": [
            "class3",
            "class5"
          ]
        }
      ]
    }
  ]
}

Second file:

{
  "files": [
    {
      "name": "some_file1",
      "methods": {
        "valid": [
          "func4",
          "func1",
          "func3"
        ],
        "invalid": [
          "func2",
          "func8"
        ]
      },
      "classes": [
        {
          "name": "class1",
          "valid": [
            "class8",
            "class5"
          ],
          "invalid": [
            "class1",
            "class2"
          ]
        }
      ]
    }
  ]
}

I get:

{
  "files": [
    {
      "methods": {
        "invalid": [
          "func2",
          "func8"
        ],
        "valid": [
          "func3",
          "func1",
          "func4"
        ]
      },
      "classes": [
        {
          "invalid": [
            "class5",
            "class3"
          ],
          "valid": [
            "class2",
            "class1"
          ],
          "name": "class1"
        }
      ],
      "name": "some_file1"
    }
  ]
}

But it's wrong because for example class5 should be valid. So my questions are:

  1. I would love to have another set of eyes to check my code and help me find out the reason for this issue.
  2. Those two methods got so complicated that it's hard to debug it. I would love to see an alternative, less complicated way to achieve it. Maybe some generic solution?

Edit: My first explanation was too complicated. I'll try to explain what I'm trying to achieve. For those of you who read the topic (appreciate it!), please forget about data type A (for simplicity). Consider Data type file B (that was showed at the start). I'm trying to merge a bunch of B files. As I understand, the algorithm for that is to do:

  • Iterate over files.
  • Check if file already located in the merged dictionary.
    • If no, we should add the file block to the files array.
    • If yes:
      • Merge methods dictionary.
      • Merge classes array.

To merge methods: method is invalid only if its invalid in both of the block. Otherwise, it's valid.

To merge classes: It's getting more complicated because it's an array. I want to follow same rule that I did for methods but I need to find the index of each block in the array, first.

The main problem is with merging classes. Can you please suggest a non-complicated on how to merge B type files?

Community
  • 1
  • 1
vesii
  • 2,760
  • 4
  • 25
  • 71
  • 1
    As a general rule, don't do `==` comparison to None. [Use `is` instead.](https://stackoverflow.com/questions/3257919/what-is-the-difference-between-is-none-and-none) And there are a number of other issues in your code (set comprehension and then casting to a list, why not a list comprehension directly?) etc. – Bram Vanroy Mar 13 '20 at 15:40
  • 2
    @dspencer unfortunately this post would currently be off-topic on CR because it isn't working as expected. Please see [this answer to _A guide to Code Review for Stack Overflow users_](https://codereview.meta.stackexchange.com/a/5778/120114) – Sᴀᴍ Onᴇᴌᴀ Mar 13 '20 at 15:50
  • 2
    Hi guys, my question is not a code review. When I ask questions before, I was to provide what I did in order to solve my problem and where I got stuck. That what I did in this topic. Now you saying that I should not do it. I'm just asking for a less complicated way to solve this issue – vesii Mar 13 '20 at 16:38
  • Seems to me you need to simplify your code and question. You don't need methods and classes, just 2 simple dictionaries each with a valid/invalid list (set?). Then we might be able to follow along. strip out everything else. Simplifying code often leads one to answer on its own as well. Once it works, put it back in your real code. – JL Peyret Mar 13 '20 at 18:53
  • Hi guys, please consider my edit. I focused the issue on B type files (without A type files). – vesii Mar 13 '20 at 21:58

1 Answers1

1

It would be great if you could provide an expected output for the example you're showing. Based on my understanding, what you're trying to achieves is:

  1. You're given multiple JSON files, each contains an "files" entry, which is a list of dictionaries with the structure:
{
  "name": "file_name",
  "methods": {
    "invalid": ["list", "of", "names"],
    "valid": ["list", "of", "names"]
  },
  "classes": [
    {
      "name": "class_name",
      "invalid": ["list", "of", "names"],
      "valid": ["list", "of", "names"]
    }
  ]
}
  1. You wish to merge structures from multiple files, so that file entries with the same "name" are merged together, according to the following rule:

    1. For each name inside "methods": if goes into "valid" if it is in the "valid" array in at least one file entry; otherwise if goes into "invalid".
    2. Classes with the same "name" are also merged together, and names inside the "valid" and "invalid" arrays are merged according to the above rule.

The following analysis of your code assumes my understanding as stated above. Let's look at the code snippet for merging lasses:

block_to_merge = self.B_report["files"][filename_index]
merged_block = {'methods': {}, 'classes': []}
for current_class in block_to_merge["classes"]:
    current_classname = current_class.get("name")
    class_index = next((index for (index, d) in enumerate(merged_block["classes"]) if d["name"] == current_classname), None)
    if class_index == None:
        merged_block['classes'] += ([{'valid': c['valid'],  'invalid': c['invalid'], 'name': c['name'] } for c in current_file['classes']])
    else:
        class_block_to_merge = merged_block["classes"][class_index]
        class_merged_block = {'valid': [], 'invalid': []}
        class_merged_block['valid'] = list(set(class_block_to_merge['valid'] + current_class['valid']))
        class_merged_block['invalid'] = list({i for l in [class_block_to_merge['invalid'], current_class['invalid']]
                                                for i in l if i not in class_merged_block['valid']})
        class_merged_block['name'] = current_classname
        merged_block["classes"][filename_index] = class_merged_block

The code is logically incorrect because:

  • You're iterating over each class dictionary from block_to_merge["classes"], which is the previous merged block.
  • The new merged block (merged_block) is initialized to an empty dictionary.
  • In the case where class_index is None, the class dictionary in merged_block is set to the the class dictionary in the previous merged block.

If you think about it, class_index will always be None, because current_class is enumerated from block_to_merge["classes"], which is already merged. Thus, what gets written into the merged_block is only the "classes" entries from the first file entry for a file. In your example, you can verify that the "classes" entry is exactly the same as that in the first file.


That said, your overall idea of how to merge the files is correct, but implementation-wise it could be a lot more simpler (and efficient). I'll first point out the non-optimal implementations in your code, and then provide a simpler solution.

  1. You're directly storing the data in its output form, however, it's not a form that is efficient for your task. It's perfectly fine to store them in a form that is efficient, and then apply post-processing to transform it into the output form. For instance:
    • You're using next to find an existing entry in the list with the same "name", but this could take linear time. Instead, you can store these in a dictionary, with "name" as keys.
    • You're also storing valid & invalid names as a list. While merging, it's converted into a set and then back into a list. This results in a large number of redundant copies. Instead, you can just store them as sets.
  2. You have some duplicate routines that could have been extracted into functions, but instead you rewrote them wherever needed. This violates the DRY principle and increases your chances of introducing bugs.

A revised version of the code is as follows:

class Merger:
    def __init__(self):
        # A structure optimized for efficiency:
        # dict (file_name) -> {
        #   "methods": {
        #     "valid": set(names),
        #     "invalid": set(names),
        #   }
        #   "classes": dict (class_name) -> {
        #     "valid": set(names),
        #     "invalid": set(names),
        #   }
        # }
        self.file_dict = {}

    def _create_entry(self, new_entry):
        return {
            "valid": set(new_entry["valid"]),
            "invalid": set(new_entry["invalid"]),
        }

    def _merge_entry(self, merged_entry, new_entry):
        merged_entry["valid"].update(new_entry["valid"])
        merged_entry["invalid"].difference_update(new_entry["valid"])
        for name in new_entry["invalid"]:
            if name not in merged_entry["valid"]:
                merged_entry["invalid"].add(name)

    def merge_file(self, src_report):
        # Method called to merge one file.
        for current_file in src_report["files"]:
            file_name = current_file["name"]
            # Merge methods.
            if file_name not in self.file_dict:
                self.file_dict[file_name] = {
                    "methods": self._create_entry(current_file["methods"]),
                    "classes": {},
                }
            else:
                self._merge_entry(self.file_dict[file_name]["methods"], current_file["methods"])
            # Merge classes.
            file_class_entry = self.file_dict[file_name]["classes"]
            for class_entry in current_file["classes"]:
                class_name = class_entry["name"]
                if class_name not in file_class_entry:
                    file_class_entry[class_name] = self._create_entry(class_entry)
                else:
                    self._merge_entry(file_class_entry[class_name], class_entry)

    def post_process(self):
        # Method called after all files are merged, and returns the data in its output form.
        return [
            {
                "name": file_name,
                "methods": {
                    "valid": list(file_entry["methods"]["valid"]),
                    "invalid": list(file_entry["methods"]["invalid"]),
                },
                "classes": [
                    {
                        "name": class_name,
                        "valid": list(class_entry["valid"]),
                        "invalid": list(class_entry["invalid"]),
                    }
                    for class_name, class_entry in file_entry["classes"].items()
                ],
            }
            for file_name, file_entry in self.file_dict.items()
        ]

We can test the implementation by:

def main():
    a = {
      "files": [
        {
          "name": "some_file1",
          "methods": {
            "valid": [
              "func4",
              "func1"
            ],
            "invalid": [
              "func3"
            ]
          },
          "classes": [
            {
              "name": "class1",
              "valid": [
                "class1",
                "class2"
              ],
              "invalid": [
                "class3",
                "class5"
              ]
            }
          ]
        }
      ]
    }
    b = {
      "files": [
        {
          "name": "some_file1",
          "methods": {
            "valid": [
              "func4",
              "func1",
              "func3"
            ],
            "invalid": [
              "func2",
              "func8"
            ]
          },
          "classes": [
            {
              "name": "class1",
              "valid": [
                "class8",
                "class5"
              ],
              "invalid": [
                "class1",
                "class2"
              ]
            }
          ]
        }
      ]
    }
    import pprint
    merge = Merger()
    merge.merge_file(a)
    merge.merge_file(b)
    output = merge.post_process()
    pprint.pprint(output)


if __name__ == '__main__':
    main()

The output is:

[{'classes': [{'invalid': ['class3'],
               'name': 'class1',
               'valid': ['class2', 'class5', 'class8', 'class1']}],
  'methods': {'invalid': ['func2', 'func8'],
              'valid': ['func1', 'func4', 'func3']},
  'name': 'some_file1'}]
Zecong Hu
  • 2,584
  • 18
  • 33