Modifying try-catch statements to reduce clunky code

Question

I've been working with SikuliX to get some try some ATDD. The code works well when only I am the one working with it. However transferring the below code to anyone else is simply counter-productive irrespective of how well I comment the code.

int numOfTries;
while (!isFinishStage && numOfTries != 3) {
                numOfTries++;
                try {
                    temp = new Pattern("imgs/img1.png").similar(0.9f);
                    s.wait(temp, 1);
                    s.find(temp);
                    s.hover(temp);
                    isFinishStage = true;
                    break;
                }catch (FindFailed ff1) {
                    try {
                    temp = new Pattern("imgs/img2").similar(0.5f);
                    s.wait(temp, 1);
                    s.find(temp);
                    s.hover(temp);
                    isFinishStage = true;
                    break;
                } catch (FindFailed ff2) {
             try{
                    temp = new Pattern("imgs/img3");
                    s.wait(temp, 1);
                    s.find(temp);
                    s.click(temp);
                } catch (FindFailed ff3) {
                    continue;
             }
        }
    }
}

A FindFailed exception is thrown once a pattern/image cannot be matched against anything on the screen (similarity simply adjusts the tolerance level). For the current GUI that it is automating, there are three possible scenarios (where this piece of code comes to play)

Screen A pops up
Screen B pops up
Neither 1 or 2, instead 'Next' pops up

Thus we check for Screen A, if not we check for Screen B, if not we check for Next, if not, repeat the cycle until we exceed the number of tries — meaning that the test has failed.

With the way Sikuli works or atleast how I've been interpreting it, you would have to perform various loops through multiple try-catch statements which seems a little off putting.

PS: The idea behind the above code is to just get it to work. If there is any ambiguity let me know so that I can clarify.

@JamesWierzba - It does, as intended, however causes significant performance drop (which I can accept at this point) and is very unfriendly to pass on to someone else to work on. — Juxhin, Jul 14 '15 at 13:56
If it works, why fix it? The code is not impossible to read. Add some comments similar to what you have said in the question. — James Wierzba, Jul 14 '15 at 14:00
@JamesWierzba - Because while the performance decrease & readability issue is acceptable at this point in time, it won't be once the script(s) get larger in size and complexity/cases. — Juxhin, Jul 14 '15 at 14:01
When code is not readable, rename things. Do not add comments to compensate for the bad naming. temp is a bad name. FindFailed is a bad name (it doesn't even end with Exception? How can you throw that?) numOfTries is a bad name (just type number). s is a terribly bad name. — Manu, Jul 14 '15 at 14:04
@Manu `FindFailed` is thrown by Sikuli when the pattern isn't found. Do agree for the other variable names though. Will be changing them — Juxhin, Jul 14 '15 at 14:06
I am not familiar with SikuliX but, can't you store your `new Pattern("imagefile")` outsite the loop? why you use `s.wait(temp, 1);` instead of directly invoking the `s.find(temp);` and `s.click(temp);`? — Paizo, Jul 14 '15 at 14:08
@Paizo - Screen#wait allows me to specify a pattern (new Pattern) and then wait a specified time for it. If I use Screen#find immediately a `FindFailed` will almost always be thrown. I will still test it without Screen#wait since I'm already catching the exception — Juxhin, Jul 14 '15 at 14:12

score 1 · Answer 1 · answered Jul 14 '15 at 14:08

1

I would like you to read software principles and make your code clean after lecture:

10 Object Oriented Design Principles

After this you should know basics like DRY and KISS principles that should emplace pretty well in your posted code.

answered Jul 14 '15 at 14:08

Paweł Głowacz

2,926
3
18
25

xyz · Answer 2 · 2015-07-14T19:03:48.913

1

Here's how:

String [] patterns = {
    "imgs/img1",
    "imgs/img2",
    "imgs/img3"
};

float [] similarities = {
    0.9f,
    0.5f,
    0.1f
};

for(int i=0; i<patterns.length; i++) {
    String str = patterns[i];
    try {     
        float sim = 0.1; // default
        try {
            sim = similarities[i];
        } catch (IndexOutofBoundsException e) {;}     
        temp = new Pattern(str).similar(sim);
        s.wait(temp, 1);
        s.find(temp);
        s.hover(temp);
        if(i != patterns.length - 1){ // Different last case
            isFinishStage = true;
            break;
        }
    } catch (FindFailed ff) {
        continue;
    }
}

edited Jul 14 '15 at 19:03

answered Jul 14 '15 at 14:14

xyz

3,349
1
23
29

I do really like this aswell, going to give it a go as well. Well done :-) – Juxhin Jul 14 '15 at 18:59
Thanks again for your time – Juxhin Jul 14 '15 at 19:21

Stephen C · Accepted Answer · 2015-07-14T14:27:52.370

The following code is (I think) equivalent to your code:

int numOfTries;
while (!isFinishStage && numOfTries < 3) {
    numOfTries++;
    if (tryPattern(s, "imgs/img1.png", 0.9f) ||
        tryPattern(s, "imgs/img2", 0.5f)) {
        isFinishStage = true;
    } else {
        // Note that the third "attempt" is inconsistent with
        // the others because you don't set isFinishedStage.
        tryPattern(s, "imgs/img3", 1.0f)
    }
}

private boolean tryPattern(SomeClass s, String path, float similarity) {
    try {
        Pattern temp = new Pattern(path);
        if (similarity != 1.0f) {
            temp = temp.similar(similarity);
        }
        s.wait(temp, 1);
        s.find(temp);
        s.hover(temp);
        return true;
    } catch (FindFailed ff) {
        return false;
    }
}

This is exactly what I was hoping for. Really like this set up and will be using the same methodology tomorrow for it. Thanks @Stephen C — Juxhin, Jul 14 '15 at 18:58

Modifying try-catch statements to reduce clunky code

3 Answers3