Java Queue data structure

Question

Could you review this implementation of a queue data structure according to the principles of Object Oriented Programming and Clean Code?

import java.util.ArrayList; public class SecurityCheckQueue<E extends ByAir> { private ArrayList<E> queue; public SecurityCheckQueue() { queue = new ArrayList<E>(); } public int getQueueLength() { return queue.size(); } public E peek() { validateLength(); return queue.get(0); } public void enqueue(E elem) { if (elem == null) throw new IllegalArgumentException("Null"); queue.add(elem); } public E dequeue() { validateLength(); return queue.remove(0); } public void validateLength() { if (getQueueLength() == 0) throw new IllegalStateException("Empty queue"); } private void checkFlight(String flight) { if (flight == null) throw new IllegalArgumentException("Null"); } private ArrayList<E> selectByFlight(String flight) { checkFlight(flight); ArrayList<E> out = new ArrayList<E>(); for (E elem : queue) { if (elem.getFlightNumber().equals(flight)) out.add(elem); } return out; } private void removeFromQueue(ArrayList<E> elements) { queue.removeAll(elements); } public void cancelFlight(String flight) { checkFlight(flight); validateLength(); ArrayList<E> elements = selectByFlight(flight); removeFromQueue(elements); } public void expedite(String flight) { checkFlight(flight); validateLength(); ArrayList<E> elements = selectByFlight(flight); removeFromQueue(elements); queue.addAll(0, elements); } public void delay(String flight) { checkFlight(flight); validateLength(); ArrayList<E> elements = selectByFlight(flight); removeFromQueue(elements); queue.addAll(elements); } } 

Answer 1

Advice 1 - Put the class in a package

It is customary in industrial code to put each class in a specific package. Perhaps you could start with this:

package com.cardstdani.flightqueue; 

Advice 2 - On type parameter name

You have:

public class SecurityCheckQueue<E extends ByAir> 

In Java, it is customary to name type parameters only using one letter, yet it make sense in your case to stick to ByAir. Just telling.

Advice 3 - On field list

You have:

private ArrayList<E> queue; 

I suggest this:

private final List<E> queue = new ArrayList<>(); 

final makes sure that you cannot accidentally reassign to queue. Plus, on the type definition of queue you can use bare interface (List) instead of implementation type (ArrayList). Finally, since Java 7, you can do diamond inference: instead of

... = new ArrayList<E>(); 

you need only

... = new ArrayList<>(); 

Advice 4 - Remove constructor

So far, we don't really need the constructor since we initalized the class object state in field initializers. Remove the constructor.

Advice 5 - On getQueueLength()

Usually, again, in Java, it is customary to call the length of something as size():

public int size() { return queue.size(); } 

Advice 6 - Throw proper exception class in validateLength

You have:

public void validateLength() { if (getQueueLength() == 0) throw new IllegalStateException("Empty queue"); } 

How about this?

import java.util.NoSuchElementException; ... public void validateLength() { if (size() == 0) { throw new NoSuchElementException("Empty queue"); } } 

(Also note the curly braces around the throw statement; it's a Java custom, too.

Advice 7 - checkFlight

private void checkFlight(String flight) { if (flight == null) throw new IllegalArgumentException("Null"); } 

Why not this?

private void checkFlight(String flight) { Objects.requireNonNull(flight, "The input flight is null."); } 

Hope that helps.

Answer 2

It seems that your class is not in fact a queue. It seems to be a domain class for some kind of an airport management system. You should describe in your questions what your code does, not just what it tries to be. This would make reviews easier (because I don't know what the code does I cannot tell if the method names need improvement and can't help you in that aspect). Please see the help centre for more info on how to ask good questions.

This leads to the main object oriented problem: your class tries to be two things at the same time: a generic queue and a domain class. This is a violation of the single responsibility principle. Your domain class should only expose methods relevant to the domain. The queue should be implemented as a standalone queue class and it's methods should be hidden from the domain API. By exposing the low level queue methods like dequeue and enqueue you may provide tools for the user to break the internal state of whatever the queue represents in the domain.

A domain class should only expose methods that are named to describe their purpose in the domain. For example enqueueFlight or whatever it is that you are queuing here.

A raw String as an identifier is error prone and reduces readability. You should introduce an identifier type, maybe FlightNumber or FlightId, to tell the user the exact data type required in the API