Kobot7 / pe

0 stars 0 forks source link

Action bar does not end after returning value #14

Open Kobot7 opened 2 months ago

Kobot7 commented 2 months ago

Problem

Refer to diagram under "The API of the following component is defined in UseCommand.java".

:CalculaChroniclesOfTheAlgorithmicKingdom makes only one execute() call to :UseCommand. However, :UseCommand is shown to return multiple times to :CalculaChroniclesOfTheAlgorithmicKingdom despite only being called once.

As indicated by the red arrow in the screenshot below, the action bar of :UseCommand does not end even after returning the first time. How is this possible?

image.png

nus-pe-script commented 2 months ago

Team's Response

Opt blocks are blocks which can either be executed or not at all. In the method execute(), there are multiple opt blocks. In each opt block, there is a return statement which exits the class, hence there is an arrow returning back to the left. If all the opt blocks are not executed, the method would then execute other methods before it returns and exits the class. This is the proper way to show multiple conditional if blocks where each if block has a return statement. It makes no sense visually if in every opt block, the UseCommand class is deactivated, following which the activation bar continues after the opt blocks with no relevant method calls to them.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: Hi team! Thanks for your explanation. Now I understand what the intended code flow is meant to be.

I agree with the team that "makes no sense visually if in every opt block, the UseCommand class is deactivated, following which the activation bar continues after the opt blocks with no relevant method calls to them". However, I believe that the team's choice of using the opt frame greatly hinders the reader.

The opt frame indicates an optional path. The implication of an optional path is that it can either be, or not be executed. Most would assume that regardless of the execution of the opt frame, the code will resume on the next step. e.g. step 1 -> step 2 (optional) -> step 3. Hence, this hinders the reader and requires additional effort to figure out what the code is doing.

In the team's ItemUsage sequence diagram, the code flow is roughly as follows:

class UseCommand extends Command {
    void execute() {
        if (inventoryIsEmpty) {
            textbox.setNextError();
            return;
        }
        if (consumableListIsEmpty) {
            textbox.setNextError();
            return;
        }
        if (itemNotSpecified) {
            textbox.setNextError();
            return;
        }
        // rest of the code logic
        return;
    }
}

The above is logically equivalent to using if-elif-else conditions:

class UseCommand extends Command {
    void execute() {
        if (inventoryIsEmpty) {
            textbox.setNextError();
            return;
        }
        else if (consumableListIsEmpty) {
            textbox.setNextError();
            return;
        }
        else if (itemNotSpecified) {
            textbox.setNextError();
            return;
        }
        else {
            // rest of the code logic
            return;
        }
    }
}

As such, it is possible use an alt frame instead of multiple opt frames. By using alt frame, it would've been immediately clear to the reader that no more than one alternative partitions be executed. Hence, preventing confusion from the multiple return statements. Example using alt frame:

image.png

Overall, I still feel that this should be considered a bug as the choice to use opt blocks instead of alt blocks hinders that user's interpretation of the program.


## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.Medium`] - [x] I disagree **Reason for disagreement:** Given the following guide for severity levels. `severity.Low`: A flaw that is unlikely to affect normal operations of the product. Appears only in very rare situations and causes a minor inconvenience only. `severity.Medium`: A flaw that causes occasional inconvenience to some readers but they can continue to use the product. I disagree that this bug causes only a **minor** inconvenience. Personally, it was hindering enough that I was left confused as to what the intended flow of program should be. Though if given a longer period of time to process this, I may have eventually understood it.