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 usingone letter, yet it make sense in your case to stick toByAir. 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 toqueue. Plus, on the type definition ofqueue you can use bare interface (List) instead of implementation type (ArrayList). Finally, since Java 7, you can dodiamond 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 - OngetQueueLength()

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

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

Advice 6 - Throw proper exception class invalidateLength

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 thethrow 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.

• 1
  \$\begingroup\$NoSuchElememtException is thrown when you try to retrieve a non-existing element. Thus it's the wrong choice in a validation method that doesn't return anything. IllegalStateException was the correct choice.\$\endgroup\$

  TorbenPutkonen

  – TorbenPutkonen

  2024-02-04 02:24:15 +00:00

  CommentedFeb 4, 2024 at 2:24

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 thehelp 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 thesingle 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 likedequeue andenqueue 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 exampleenqueueFlight or whatever it is that you are queuing here.

A rawString as an identifier is error prone and reduces readability. You should introduce an identifier type, maybeFlightNumber orFlightId, to tell the user the exact data type required in the API

• java
• object-oriented