3
\$\begingroup\$

I wrote the following function which coverts a JSON format into a CSV. I'll be glad to have a code review because I know that it could be done in a simpler way and with less lines and iterations. Before I will show you the code, I will explain what it does. The format of JSON:

[
  {
    "subs": [
      {
        "status": "Passed",
        "name": "sub1",
        "all_data": [
          {
            "status": "Passed",
            "name": "prepare"
          },
          {
            "status": "Passed",
            "name": "run_check"
          }
        ],
        "ward": "/disk/nex/sub1/tes2"
      },
      {
        "status": "Passed",
        "name": "sub2",
        "all_data": [
          {
            "status": "Passed",
            "name": "prepare"
          },
          {
            "status": "Passed",
            "name": "run_check"
          },
          {
            "status": "Passed",
            "name": "analyze_command"
          }
        ],
        "ward": "/disk/nex/sub2/test1"
      }
    ],
    "name": "Gandif/test1"
  },
  {
    "subs": [
      {
        "status": "Passed",
        "name": "checker",
        "all_data": [
          {
            "status": "Passed",
            "name": "prepare"
          },
          {
            "status": "Passed",
            "name": "analyze_command"
          }
        ],
        "ward": "/disk/nex/checker/tes2"
      }
    ],
    "name": "Gandif/test2"
  }
]

I want to cover this data into a CSV. I have created the following checkbox values so the user could choose which data the CSV should contain:

checkboxValues: {
    csv_title:   {value: true, name: "CSV Title"},
    main_name:   {value: true, name: "Main Name"},
    sub_ward:    {value: true, name: "Subtest Ward"},
    sub_name:    {value: true, name: "Subtest Name"},
    sub_status:  {value: true, name: "Subtest Status"},
    data_name:   {value: true, name: "Data Name"},
    data_status: {value: true, name: "Data Status"}
}

The format of the csv file:

Main Name, Subtest Ward, Subtest Name, Subtest Status, Data Name, Data Status, Data Name, Data Status, Data Name, Data Status, ...

User could use the checkbox values to choose which data should be included in the CSV file.

The final result for the above example should be (if all checkboxs are checked):

[
  [
    "Main Name",
    "Subtest Ward",
    "Subtest Name",
    "Subtest Status",
    "Data Name",
    "Data Status",
    "Data Name",
    "Data Status",
    "Data Name",
    "Data Status"
  ],
  [
    "Gandif/test1",
    "/disk/nex/fub1/tes2",
    "fub1",
    "Passed",
    "prepare",
    "Passed",
    "run_check",
    "Passed"
  ],
  [
    "Gandif/test1",
    "/disk/nex/fub2/test1",
    "fub2",
    "Passed",
    "prepare",
    "Passed",
    "run_check",
    "Passed",
    "analyze_command",
    "Passed"
  ],
  [
    "Gandif/test2",
    "/disk/nex/checker/tes2",
    "checker",
    "Passed",
    "prepare",
    "Passed",
    "analyze_command",
    "Passed"
  ]
]

The function that coverts the JSON to a CSV format looks like:

JSO2CSV: function(jsondata) {
    if (typeof jsondata !== "undefined" && jsondata) {
        let arr = [];
        let temp_arr = [];
        let totalAmount = 0;
        jsondata.forEach(main => {
            main.subs.forEach(sub => {
                let inner_array = [];
                if (this.checkboxValues.main_name.value)  inner_array.push(main.name  || 'N/A');
                if (this.checkboxValues.sub_ward.value)   inner_array.push(sub.ward   || 'N/A');
                if (this.checkboxValues.sub_name.value)   inner_array.push(sub.name   || 'N/A');
                if (this.checkboxValues.sub_status.value) inner_array.push(sub.status || 'N/A');
                sub.all_data.forEach(data => {
                    if (this.checkboxValues.data_name.value)   inner_array.push(data.name   || 'N/A');
                    if (this.checkboxValues.data_status.value) inner_array.push(data.status || 'N/A');
                });
                totalAmount = (totalAmount < sub.all_data.length) ? sub.all_data.length : totalAmount;
                temp_arr.push(inner_array);
            });
        });
        let temp = [];
        let result = [];
        for (var i = 0; i < totalAmount; i++) {
            if (this.checkboxValues.data_name.value)   temp.push(this.checkboxValues.data_name.name);
            if (this.checkboxValues.data_status.value) temp.push(this.checkboxValues.data_status.name);
        }
        Object.values(this.checkboxValues).map(box => {
            if (box.name == this.checkboxValues.data_name.name) {
                result = result.concat(temp);
            } else if (box.name == this.checkboxValues.csv_title.name) {

            } else if (box.name != this.checkboxValues.data_status.name && box.value) {
                result.push(box.name);
            }
        });

        arr.push(result);
        arr = arr.concat(temp_arr);
        return arr;
    }
    return [];
}

It looks like my code is working but I'm trying to make it look more professional and to have better logic. For example, I'm defining too many temp arrays. Also the for loop probably could be done better. I'll be more then glad to see a review for this function.

\$\endgroup\$
1
\$\begingroup\$

Couple of things right off the bat... Your first if statment should be a catch and return. like

if (typeof jsondata == "undefined" || !jsondata) {
   return [];
}

That will keep the rest of the code with one less indent.

You have a stray else if satement not doing anything.

else if (box.name == this.checkboxValues.csv_title.name) {

}

I think your temp arrays are too ambiguous, you should at least comment what they are for.

I see no reason why the contents of this for loop

for (var i = 0; i < totalAmount; i++) {
            if (this.checkboxValues.data_name.value)   temp.push(this.checkboxValues.data_name.name);
            if (this.checkboxValues.data_status.value) temp.push(this.checkboxValues.data_status.name);
        }

cannot be nested inside main.subs.forEach loop. That will reduce the amount of loops you have by 1.

Overall I have a feeling you are making this way more complicated then it has to be. I would take a look at the code for your checkboxes and see if there's an easier way to relay that information into your Typescript.

Ultimately you should end up with something like this

JSO2CSV: function(jsondata) {
    if (typeof jsondata == "undefined" || !jsondata)
        return [];

    let boxes = this.checkboxValues;
    let header = [];
    let result = []; //final array

    Object.values(boxes).map(box => {
        if(box.value)
            header.push(box.name)
    });

    jsondata.forEach(main => {
        main.subs.forEach(sub => {
            let subarr = [];
            //Build sub array
            if (boxes.main_name.value)  
                subarr.push(main.name  || 'N/A');
            if (boxes.sub_ward.value)
                subarr.push(sub.ward   || 'N/A');
            if (boxes.sub_name.value)
                subarr.push(sub.name   || 'N/A');
            if (boxes.sub_status.value)
                subarr.push(sub.status || 'N/A');
            sub.all_data.forEach(data => {
                if (boxes.data_name.value)
                    subarr.push(data.name   || 'N/A');
                if (boxes.data_status.value)
                    subarr.push(data.status || 'N/A');
            });
            result.push(subarr);
            //Add to header if it's too short
            while(header.length < sub.all_data.length - header.length){
                if (boxes.data_name.value)
                    header.push(boxes.data_name.name);
                if (boxes.data_status.value)
                    heaer.push(boxes.data_status.name);
            }
        });
    });

    result.unshift(header);

    return result;
}

This is sudo code, but you can get a feel.

\$\endgroup\$
8
  • \$\begingroup\$ Thank you for your review. Can you please add on how should I build the header before the forEach loop if I need to calculate totalAmount . \$\endgroup\$ – vesii Oct 16 '19 at 19:23
  • \$\begingroup\$ Sure, give me a minute. \$\endgroup\$ – Rager Oct 16 '19 at 19:27
  • \$\begingroup\$ Updated to include header. Simplified the .map but it looks to me like that's what you were going for. \$\endgroup\$ – Rager Oct 16 '19 at 19:55
  • \$\begingroup\$ have you tested it? it looks like define sub twice \$\endgroup\$ – vesii Oct 16 '19 at 20:21
  • \$\begingroup\$ Good catch, no I have not tested it. \$\endgroup\$ – Rager Oct 16 '19 at 20:25

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.