Do you want to count collections or know if they are empty?
TL;DR: Use declarative names. Always
Problems
Readability
Cache Invalidation
Performance Penalties
Type Safety
Solutions
- Replace count() == 0 and size()==0 usages
Context
isEmpty() and count()==0 seem to be equivalent but have deep differences.
The semantics are clearer.
Skipping this declarative method violating the collection encapsulation might have performance issues.
Sample Code
Wrong
import java.util.EmptyStackException;
import java.util.Stack;
public class SchrodingerStack<T> {
private Stack<T> stack;
public SchrodingerStack() {
stack = new Stack<>();
}
public void push(T item) {
stack.push(item);
}
public T pop() {
if (stack.size() == 0) {
throw new EmptyStackException();
}
T item = stack.pop();
return item;
}
public int size() {
return stack.size();
// This has O(n) linear time
// And the stack might not be fully reachable in memory
// While you wait, the stack isEmpty and notEmpty
// at the same time
}
public static void main(String[] args) {
SchrodingerStack<String> stack = new SchrodingerStack<>();
stack.push("Siamese");
stack.push("Garfield");
while (stack.size() > 0) {
System.out.println("Popped element: " + stack.pop());
}
if (stack.size() == 0 ) {
// Less readable
// violating encapsulation
// and coupled to the implementation
System.out.println("The stack is empty.");
} else {
System.out.println("The stack is not empty.");
}
}
}
Right
import java.util.EmptyStackException;
import java.util.Stack;
public class SchrodingerStack<T> {
private Stack<T> stack;
private boolean isEmpty;
public SchrodingerStack() {
stack = new Stack<>();
isEmpty = true;
}
public void push(T item) {
stack.push(item);
isEmpty = false;
}
public T pop() {
if (isEmpty()) {
throw new EmptyStackException();
}
T item = stack.pop();
if (stack.isEmpty()) {
isEmpty = true;
}
return item;
}
public boolean isEmpty() {
return isEmpty;
// This has O(1) constant time
}
public int size() {
return stack.size();
// This has O(n) linear time
// And the stack might not be fully reachable in memory
// While you wait, the stack isEmpty and notEmpty
// at the same time
}
public static void main(String[] args) {
SchrodingerStack<String> stack = new SchrodingerStack<>();
stack.push("Siamese");
stack.push("Garfield");
while (!stack.isEmpty()) {
System.out.println("Popped element: " + stack.pop());
}
if (stack.isEmpty()) {
// Semantic operation not violating encapsulation
System.out.println("The stack is empty.");
} else {
System.out.println("The stack is not empty.");
}
}
}
Detection
[X] Automatic
You can check for this expression using syntax abstraction trees.
Tags
- Readability
Level
[X] Beginner
AI Generation
LLMs generate abstractions using empty() functions
AI Detection
Gemini detected the problem of using count() == 0
Conclusion
Using IsEmpty() is recommended for checking if a collection is empty due to its clarity and potential performance benefits.
Relations
Code Smell 233 - Collections Count
Maxi Contieri ・ Dec 1 '23
Disclaimer
Code Smells are my opinion.
Credits
Photo by Valentin Lacoste on Unsplash
Good programming is good writing.
John Shore
Software Engineering Great Quotes
Maxi Contieri ・ Dec 28 '20
This article is part of the CodeSmell Series.
Top comments (5)
I definitely agree with your rationale of clarity for using
isEmpty()
. However, there is no performance difference. Java'sStack
class extendsVector
, which maintains number of elements in an instance field, and thesize()
method is a simple getter with an O(1) runtime. Your O(n) comment in examples is incorrect. There isn't any performance difference because the same operations are done (theisEmpty()
method checks the same field that thesize()
method returns). The benefit ofisEmpty()
is purely readability, which of course is sufficient motivation.Your example of the "right" way is actually duplicating functionality that is already in Java's
Stack
class. YourisEmpty()
should just callstack.isEmpty()
and you can eliminate theisEmpty
field and all other logic associated with it.Yeah, this whole example is questionable.
Might be beside the point, but I don't know why would anybody use
Stack
orVector
in modern Java code. The recommended way for a stack in Java would be to useDeque
with an implementation likeArrayDeque
orLinkedList
. You might prefer the first one. Both of these implement theisEmpy
method inherited fromCollection
interface. Both implementation run in O(1) time.Nevertheless, I agree with a premise of this post. I think it is a good practice to have a method for checking the emptiness of a collection.
One reason someone might prefer
Stack
over one of theDeque
implementations is in a context where you need a LIFO data structure. Someone reading code that is using aStack
immediately knows it is LIFO. With aDeque
since removals and additions can be at both ends, you need more context to determine how it is being used. Is it being used like a stack, or a queue, or as its name implies like a double-ended queue?Stack
expresses intent more clearly thanDeque
in a LIFO context.If you
pop
an item, it'll call theisEmpty()
method, which returns the class variableisEmpty
. If that'strue
, then it'll set the class variable totrue
. Isn't that tautological?It feels like the
isEmpty()
method should wrap thesize
orcount
you're trying to avoid, else this won't work.Thank you very much
The main challenge is about readabality and not performance.
I'm not an expert in Java and I didn't know this facts so they are good points