1

I tried this but it says this error:

cannot resolve symbol i

public Incident getNextIncident() {
    if (!incidentQueue.isEmpty()) {
        Incident i = incidentQueue.element();
        incidentQueue.remove();
    }
    return i;
}
Zabuzard
  • 25,064
  • 8
  • 58
  • 82
David Shu
  • 29
  • 3
  • What do you expect `i` to be when `incidentQueue` is empty? – khelwood Oct 07 '19 at 08:03
  • 1
    Because you are trying to use variable `i` out of its scope. – Amit Bera Oct 07 '19 at 08:04
  • The variable `i` is only in scope inside the `if` block. It doesn't exist anymore when you try to return it. What class is `incidentQueue`? – Kayaman Oct 07 '19 at 08:04
  • You can not use a variable outside of the scope it was declared in. You create `i` inside the `if`, so it will be destroyed as soon as the `if` ends. You have to create `i` before the `if`. For example as `Incident i = null;` and then assign it inside the `if` as `i = ...`. Then you can use it outside. But you need to be aware that it is `null` if it did not enter the `if`, you should handle this case. – Zabuzard Oct 07 '19 at 08:05
  • So do I need the i? And how to return the Incident? – David Shu Oct 07 '19 at 08:09
  • And if I change the return type from Incident to void. Would the remove method still return the Incident? incidentQueue is a Queue. – David Shu Oct 07 '19 at 08:11
  • It is impossible to return a value in a `void` method. `void` literally means _"I do not return anything"_. – Zabuzard Oct 07 '19 at 08:12

4 Answers4

2

Explanation

You can not use a variable outside of the scope it was declared in.

You create i inside the if, so it will be destroyed as soon as the if ends. You have to create i before the if. For example:

public Incident getNextIncident() {
    Incident i = null;
    if (!incidentQueue.isEmpty()) {
        i = incidentQueue.element();
        incidentQueue.remove();
    }
    return i;
}

Then you can use it outside, as you see.


Handle empty case

But you need to be aware that it is null if it did not enter the if, you should handle this case:

public Incident getNextIncident() {
    Incident i = null;
    if (!incidentQueue.isEmpty()) {
        i = incidentQueue.element();
        incidentQueue.remove();
    }

    if (i == null) {
        // TODO Handle this case ...
    }

    return i;
}

Note that you can also directly return from inside your if, there is no need to do it outside:

public Incident getNextIncident() {
    if (!incidentQueue.isEmpty()) {
        Incident i = incidentQueue.element();
        incidentQueue.remove();
        return i;
    }

    // TODO Handle this case ...
}

Or, slightly rewritten in an early-return fashion (preferred style):

public Incident getNextIncident() {
    if (incidentQueue.isEmpty()) {
        // TODO Handle this case ...
    }

    Incident i = incidentQueue.element();
    incidentQueue.remove();
    return i;
}

Poll

Note that there is a commonly used name for a method in a queue that removes and gives back the first value, poll. And if your queue happens to be a queue from the Java library, as opposed to a custom class, it also already has such a method.

See the documentation of Queue#poll:

Retrieves and removes the head of this queue, or returns null if this queue is empty.

So instead of creating this method, you could just do

Incident head = incidentQueue.poll();
// instead of
Incident head = incidentQueue.getNextIncident();

Optional

Let me propose you a modern way of handling the case that you can not return a value because the queue is empty.

The old-style way is to return null, but this has many drawbacks. Since Java 8, we have Optional which was created exactly for that (see its documentation).

public Optional<Incident> poll() {
    if (incidentQueue.isEmpty()) {
        return Optional.empty();
    }

    Incident head = incidentQueue.element();
    incidentQueue.remove();
    return Optional.of(head);
}

Note

I would also suggest to rename the incident variable to incident or head instead of i. Do not abbreviate variable names, except they are commonly known (like http) or it is common pattern (like i, j, k for loops or a, b for math operations). i would only be confusing here, as it is usually used as index variable int i in a loop.

Zabuzard
  • 25,064
  • 8
  • 58
  • 82
  • Thanks. But is it possible for the method to do nothing if the Queue is empty? – David Shu Oct 07 '19 at 08:20
  • You have to return _something_ then. Old-fashion would be to return `null`. Then the user can check this case `if (foo == null) ...`. Modern style would be to use `Optional`. That's exactly what it was made for. So your return type is `Optional` and you return `Optional.empty()` then. And if there is a value, you return `Optional.of(incident)`. Then the user can check using `result.isEmpty()` and similar methods. (Updated the answer) – Zabuzard Oct 07 '19 at 08:23
1

You're duplicating existing Queue functionality for nothing. Your code (if it worked) is equivalent to

public Incident getNextIncident() {
    return incidentQueue.poll();
}
Kayaman
  • 72,141
  • 5
  • 83
  • 121
0

You are declaring i inside an if condition, so the compiler are saying that in the line return i; it may not know what is i.

You must declare it before the if:

public Incident getNextIncident() {
    Incident i = null;
    if (!incidentQueue.isEmpty()) {
        i = incidentQueue.element();
        incidentQueue.remove();
    }
    return i;
}
user2342558
  • 5,567
  • 5
  • 33
  • 54
0

It should be just a small issue. You have declared the Incident object inside the if block. Meaning it only stays there. After you reach the return i; part, it basically doesn't exist. You have to declare the Incident i outside the if block.

pavleprica
  • 76
  • 4