2
\$\begingroup\$

I designed a java class to represent a live well (fishing). Looking for help with how I can make this more efficient, or if anyone sees where I can improve on the design, it would be appreciated.

package com.livewell.model;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import org.apache.commons.lang.StringUtils;

/**
 * Represents a live well on a fishing boat. Fish can be added to the live well. As fish are added,
 * only the largest n fish are kept, where n = the defined capacity. The BigFishFactor is used when
 * determining which fish is largest.
 *
 */
public class LiveWell {

    //default values
    private static final int defaultCapacity = 3;
    private static final int defaultMinimumFishLengthInInches = 12;
    private static final int defaultMinimumFishWeightInOunces = 6;
    private static final BigFishFactor defaultBigFishFactor = BigFishFactor.WEIGHT;

    //live well properties
    private final int capacity;
    private final int minimumFishLengthInInches;
    private final int minimumFishWeightInOunces;
    private final BigFishFactor bigFishFactor;

    //live well contents
    private List<Fish> contents = new ArrayList<Fish>();

    /**
     * Constructs a live well using the default values for minimum length, weight, and big fish factor
     */
    public LiveWell(){
        this(null, null, null, null);
    }

    /**
     * Constructs a live well.
     * @param capacity maximum capacity of the live well
     * @param minimumFishLengthInInches minimum length of a fish that will be accepted for addition to the live well
     * @param minimumFishWeightInOunces minimum weight of a fish that will be accepted for addition to the live well
     * @param bigFishFactor factor used when comparing 2 fish (for purposes of determining which fish will be kept)
     */
    public LiveWell(Integer capacity, Integer minimumFishLengthInInches, Integer minimumFishWeightInOunces, BigFishFactor bigFishFactor){
        this.capacity = capacityValidOrProvided(capacity) ? capacity : defaultCapacity;
        this.minimumFishLengthInInches = lengthValidOrProvided(minimumFishLengthInInches) ? minimumFishLengthInInches : defaultMinimumFishLengthInInches;
        this.minimumFishWeightInOunces = weightValidOrProvided(minimumFishWeightInOunces) ? minimumFishWeightInOunces : defaultMinimumFishWeightInOunces;
        this.bigFishFactor = bigFishFactor == null ? defaultBigFishFactor : bigFishFactor;
    }

    private static boolean capacityValidOrProvided(Integer capacity){
        return capacity != null && capacity >= 0;
    }

    private static boolean lengthValidOrProvided(Integer minimumFishLengthInInches){
        return minimumFishLengthInInches != null && minimumFishLengthInInches >= 0;
    }

    private static boolean weightValidOrProvided(Integer minimumFishWeightInOunces){
        return minimumFishWeightInOunces != null && minimumFishWeightInOunces >= 0;
    }

    public int getNumberOfFish(){
        return contents.size();
    }

    public List<Fish> getContents(){
        return Collections.unmodifiableList(contents);
    }

    /**
     * May add a fish to the live well.
     * @param fishLengthInInches the length of the fish being added to the live well
     * @param fishWeightInOunces the weight of the fish being added to the live well
     * @return true if and only if all of the following conditions are satisfied:
     * 
     * -the capacity of the live well is > 0
     * -the length and weight of the fish are >= the minimums defined
     * -the fish is larger than the smallest fish currently in the live well (where larger is defined
     *  by the BigFishFactor)
     */
    public boolean addFishToLiveWell(int fishLengthInInches, int fishWeightInOunces){
        boolean fishAddedToLiveWell = false;
        boolean fishIsKeeper = fishLengthInInches >= minimumFishLengthInInches && fishWeightInOunces >= minimumFishWeightInOunces;
        if(fishIsKeeper){
            Fish fishToAdd = new Fish(fishLengthInInches, fishWeightInOunces);
            int numberOfFish = getNumberOfFish();
            //if we have reached capacity
            if(numberOfFish == capacity){
                if(capacity > 0){
                    //if there are fish
                    if(numberOfFish > 0){
                        //if bigger than the smallest fish, replace the smallest fish
                        Fish smallestFish = contents.get(0);
                        if(fishToAdd.compareTo(smallestFish) > 0){
                            _replaceSmallestFish(fishToAdd);
                            fishAddedToLiveWell = true;
                        }
                    }
                    //if there are no fish, add the fish
                    else{
                        _addFishToLiveWell(fishToAdd);
                        fishAddedToLiveWell = true;
                    }
                }
            }
            //if we are not at capacity, add the fish
            else{
                _addFishToLiveWell(fishToAdd);
                fishAddedToLiveWell = true;
            }
        }
        return fishAddedToLiveWell;
    }

    private void _replaceSmallestFish(Fish fishToReplaceSmallestFishWith){
        contents.remove(0);
        _addFishToLiveWell(fishToReplaceSmallestFishWith);
    }

    private void _addFishToLiveWell(Fish fishToAdd){
        contents.add(fishToAdd);
        Collections.sort(contents);
    }

    public class Fish implements Comparable<Fish> {
        private int lengthInInches;
        private int weightInOunces;

        public Fish(int lengthInInches, int weightInOunces){
            this.lengthInInches = lengthInInches;
            this.weightInOunces = weightInOunces;
        }

        public int getLengthInInches() {
            return lengthInInches;
        }

        public int getWeightInOunces() {
            return weightInOunces;
        }

        @Override
        public int compareTo(Fish o) {
            int compare = 0;
            if(o != null){
                switch(bigFishFactor){
                case COMBO:
                    compare = new Integer(lengthInInches * weightInOunces).compareTo(new Integer(o.lengthInInches * o.weightInOunces));
                    break;
                case LENGTH:
                    compare = new Integer(lengthInInches).compareTo(o.lengthInInches);
                    break;
                case WEIGHT:
                    compare = new Integer(weightInOunces).compareTo(o.weightInOunces);
                    break;
                }
            }
            else{
                compare = -1;
            }
            return compare;
        }

        @Override
        public String toString(){
            return lengthInInches+"in, "+weightInOunces+"oz";
        }

    }

    /**
     * Factor used when determining which of 2 fish will be kept in the live well (which is 'larger')
     */
    public enum BigFishFactor{
        WEIGHT("Fish with the highest weight is deemed to be the largest."),
        LENGTH("Fish with the longest length is deemed to be the largest."),
        COMBO("Fish with the largest (length * width) is deemed to be the largest.");

        private String displayValue;

        private BigFishFactor(String displayValue){
            this.displayValue = displayValue;
        }

        public String getDisplayValue() {
            return displayValue;
        }

    }

    @Override
    public String toString(){
        StringBuilder sb = new StringBuilder();
        sb.append("Maximum Capacity: ").append(capacity);
        sb.append("\n");
        sb.append("Minimum Length: ").append(minimumFishLengthInInches).append(" in.");
        sb.append("\n");
        sb.append("Minimum Weight: ").append(minimumFishWeightInOunces).append(" oz.");
        sb.append("\n");
        sb.append("Big Fish Determining Factor: ").append(bigFishFactor.getDisplayValue());
        sb.append("\n");
        sb.append("*****Live Well Contents****");
        sb.append("\n");
        if(contents.size() == 0){
            sb.append("Live Well Empty");
        }else{
            sb.append(StringUtils.join(contents, "\n"));
        }
        sb.append("\n");
        sb.append("***************************");
        return sb.toString();
    }

}
\$\endgroup\$
  • \$\begingroup\$ What is your definition of "efficient"? \$\endgroup\$ – Timothy Truckle Feb 19 '17 at 10:13
3
\$\begingroup\$

if anyone sees where I can improve on the design, it would be appreciated.

use of null reference.

A null should only be used if it is a valid element of the value set. (which in consequence does not need special treatment withing the unit...)

But in your case it is an "in band signal" that the input is invalid.

And you do this without any need because instead of calling the "null handling" static methods from the parameterized constructor you could simply pass the default values in from the parameterless constructor instead of the nulls.

You may argue that you have to call that "null handling" static methods anyway because you want the user to be able to pass nulls. But this it a misconception of "simple API". Instead of letting the user passing in nulls you should provide the default values as public constants which can be passed in by the user. This way the user knows that default values are used by reading her own code. When passing nulls she needs to look up your code (at least your javadoc) to verify that.

naming conventions

You use underscore to prefix private (member) methods. The Java Naming Conventions implicitly discourage that. But if you decide to do so you should do it consequently and also use prefixes for the static private methods too...

immutability

Your class Fish does only have getter. Therefor I assume it is meant to be immutable. So why don't you express that by making the properties of a fish final?

primitive obsession / separation of concerns

Your public method addFishToLiveWell() has two primitive parameters which are properties of a Fish object yet to build. Since your class Fish is public why not making it clear to the user that she is adding a fish by having a single parameter of type Fish? why should the creation of a fish object be the responsibility of this method?

feature envy / separation of concerns

In your method addFishToLiveWell() you check the properties of a fish before creating it.

Why is this a responsibility of this method?

Since class Fish is public shouldn't this check also be available for users of your code?

know your tools

You use an ArrayList to hold the fishes.
You keep track of the order of fishes in that list yourself.

If I would implement that I'd use a TreeSet which orders the content automatically either by the given Comparator implementation or the elements implementation of the Comparable interface.

Doing so the addFishToLiveWell() method would change to this:

public boolean addFishToLiveWell(Fish newfish){
    contents.add(newfish);
    if(capacity < contents.size()){
        Fish smallestFishInTank = contents.pollLast();
        return smallestFishInTank == newfish;
    }
    return true;
}

inconsequent use of Java enums

You created a Java enum class and you use methods on that enum.

But then you use this enum to to branch the execution flow with a switch statement. Why not putting the actual compare operation into the enum?

public enum BigFishFactor {
    WEIGHT("Fish with the highest weight is deemed to be the largest.") {
        @Override
        int compare(Fish f1, Fish f2) {
            return new Integer(f1.getWeightInOunces()).compareTo(f2.getWeightInOunces());
        }

    },
    LENGTH("Fish with the longest length is deemed to be the largest.") {
        @Override
        int compare(Fish f1, Fish f2) {
            return new Integer(f1.getLengthInInches()).compareTo(f2.getLengthInInches());
        }
    },
    COMBO("Fish with the largest (length * width) is deemed to be the largest.") {
        @Override
        int compare(Fish f1, Fish f2) {
            return new Integer(f1.getLengthInInches() * f1.getWeightInOunces())
                    .compareTo(new Integer(f2.getLengthInInches() * f2.getWeightInOunces()));
        }
    };

    private String displayValue;

    private BigFishFactor(String displayValue) {
        this.displayValue = displayValue;
    }

    public String getDisplayValue() {
        return displayValue;
    }

    abstract int compare(Fish f1, Fish f2);
}

and in class Fish:

    @Override
    public int compareTo(Fish o) {
        int compare = 0;
        if (o != null) {
                compare = bigFishFactor.compare(this, o);
        } else {
            compare = -1;
        }
        return compare;
    }

redundant / lying comments

These comments do not convey any additional information:

// default values
// live well properties
// live well contents

This comment actually is a lie into the face of the reader:

/**
 * Constructs a live well using the default values for minimum length,
 * weight, and big fish factor
 */

The following code is passing nulls, not default values...

identifier names

While names of variable should start with nouns in general there is an exception for variables holding booleans: they should strt with is or have as well as method that return a boolen.

also you have a method addFishToLiveWell. It is located in a class named LiveWell which makes this part of the name obsolete. Also it is supposed to add a Fish object (at least after my refactoring suggestion). Therefor this part of the name is also obsolete.

If you want to keep the two-parameter version you could keep the Fish part but change the name to:

 addFishWith(int lengthInInches, int weightInOunces);
\$\endgroup\$
  • \$\begingroup\$ I'm not sure if I see your point about the constructor, though. The way I have it written, the no arg constructor will effectively construct a live well that uses the default values. Yes, nulls are passed to the 2nd constructor as arguments, but that results in a livewell object with the default values. I could be missing your point on that one, my apologies if so. \$\endgroup\$ – user1154644 Feb 20 '17 at 3:33
  • \$\begingroup\$ @user1154644: the point is: setting up the default values is a side effect of the parameterized constructor, but you should encurage passing explicit values to it which meight be the defaults. And the parameterless constructor should uses this defaults to call the other one. \$\endgroup\$ – Timothy Truckle Feb 20 '17 at 7:08
  • \$\begingroup\$ I see your point. Thanks for your assistance and input! \$\endgroup\$ – user1154644 Feb 20 '17 at 18:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.