1
\$\begingroup\$

The code is to check the rows in a table. If in a table two non consecutive rows are checked then all the in between rows should be selected by itself. Also, after selecting two rows, on selection of third row it should throw an error.

How can I simplify this code?

onSelectionChange: function(oEvent) {
        var aIndex = [];
        var k = 0;
        var z = 0;
        var sItem;
        var sContext;
        var vMatched;
        if (oEvent.getParameters().listItems[0].getSelected() === true) {
            if (oEvent.getParameters().selectAll === true) {
                for (var a = 0; a < this.getView().byId("monTable").getItems().length; a++) {
                    aIndex.push(a);
                }
            } else {
                for (k = 0; k < this.getView().byId("monTable").getItems().length; k++) {
                    sItem = this.getView().byId("monTable").getItems()[k].getBindingContext().sPath;
                    for (z = 0; z < this.getView().byId("monTable").getSelectedContextPaths().length; z++) {
                        sContext = this.getView().byId("monTable").getSelectedContextPaths()[z];
                        if (sItem === sContext) {
                            aIndex.push(k);
                        }
                    }
                }
            }
        } else {
            for (k = 0; k < this.getView().byId("monTable").getItems().length; k++) {
                sItem = this.getView().byId("monTable").getItems()[k].getBindingContext().sPath;
                for (z = 0; z < this.getView().byId("monTable").getSelectedContextPaths().length; z++) {
                    sContext = this.getView().byId("monTable").getSelectedContextPaths()[z];
                    if (sItem) {
                        vMatched = k;
                    }
                }
            }
            if (oEvent.getParameters().selectAll === false && this.getView().byId("monTable").getSelectedContextPaths().length === 0) {
                aIndex.splice(0, aIndex.length);
            } else {
                for (var j = 0; j < aIndex.length; j++) {
                    if (aIndex[j] === vMatched) {
                        aIndex.splice(j, 1);
                    }
                }
            }
        }
        var maxValue = aIndex[0];
        var minValue = aIndex[0];
        for (var i = 0; i < aIndex.length; i++) {
            if (aIndex[i] > maxValue) {
                maxValue = aIndex[i];
            }
            if (aIndex[i] < minValue) {
                minValue = aIndex[i];
            }
        }
        if (maxValue - minValue > 1) {
            if (aIndex.length > 2 && aIndex.length !== this.getView().byId("monTable").getItems().length) {
                for (var y = minValue; y <= maxValue; y++) {
                    if (this.getView().byId("monTable").getItems()[y].getSelected() === false) {
                        var sText = this.getOwnerComponent().getModel("i18n").getResourceBundle().getText("msgTxtgapSelection");
                        var sTitle = this.getOwnerComponent().getModel("i18n").getResourceBundle().getText("msgTitleInf");
                        this.dynamicNoneMessage(sText, sTitle);
                        //MessageBox.show("Bitte wählen Sie eine Folge ohne Lücken");
                    }

                }

            } else {
                for (var m = minValue; m <= maxValue; m++) {
                    this.getView().byId("monTable").getItems()[m].setSelected(true);

                }

            }
        }
    }` 
\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$ – Martin R Sep 5 at 11:46
1
\$\begingroup\$

Easiest thing you can do is properly name your variables and add comments.

Seeing things like this is very frustrating:

aIndex.push(k);
vMatched = k;

It's completely meaningless except to the developer who wrote it. It's a form of hungarian notation, except nothing is preventing you from making aIndex a String instead of an Array or vMatched and array instead of a var. (I'm assuming v is for var, which is also a hint this naming style isn't going to work here).

I'd expect for-loops to start using i not k. No need to declare it at the top either. You should always declare variables in the most local scope.

Here you're adding the numbers 0-a onto a list. You don't need to do that since you already have a. If you need to store the length at that time, you can.

for (var a = 0; a < this.getView().byId("monTable").getItems().length; a++) {
    aIndex.push(a);
}

In your nested loops it looks like you are getting the indexes that match based on a criteria. You should really do this in a method, hopefully with comments to make it easier to read. You can use lambdas expressions if you're comfortable with them. However without better context I cannot provide a concrete alternative.

You should use a method for getting the min/max amounts from an array. Methods make everything easier to read & easier to refactor.

You can use Arrays.sort() then take the first / last item of the array to get the min/max.

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