List Filtering Code

Question

I am developing applications for android. In my application, the user can add information about his weight and watch the progress of weight change. I decided to add filtering of the elements according to the following criteria: the presence of a comment, the absence of a comment, weight gain, weight reduction. For this, I wrote the WeightFilterUseCase and WeightFilterUseCaseTest classes for testing. Could you talk about the flaws of my code?

WeightFilterUseCase

 public class WeightFilterUseCase {

   public List<WeightUI> filter(List<WeightUI> weights, WeightFilter weightFilter) {
    switch (weightFilter) {
        case ShowOnlyHaveComment:
            return filterByEmptyComment(weights, true);
        case ShowOnlyHaventComment:
            return filterByEmptyComment(weights, false);
        case ShowOnlyWeightGrowing:
            return filterWeightByChange(weights, WeightChange.WeightGrowing);
        case ShowOnlyWeightDecreases:
            return filterWeightByChange(weights, WeightChange.WeightDecreases);
    }
    throw new EnumConstantNotPresentException(WeightFilter.class, weightFilter.toString());
  }

  private List<WeightUI> filterByEmptyComment(List<WeightUI> weights, boolean isEmpty) {
    return weights.stream().filter(weight -> weight.getComment().trim().isEmpty() == isEmpty).collect(Collectors.toList());
  }

  private List<WeightUI> filterWeightByChange(List<WeightUI> weights, WeightChange weightChange) {
    return weights.stream().filter(weight -> weight.isWeightGrowing() == weightChange).collect(Collectors.toList());
  }

  public enum WeightFilter {
    ShowOnlyHaveComment,
    ShowOnlyHaventComment,
    ShowOnlyWeightGrowing,
    ShowOnlyWeightDecreases
  }
}

WeightFilterUseCaseTest

public class WeightFilterUseCaseTest {
    private List<WeightUI> weights = new ArrayList<>();
    private WeightFilterUseCase weightFilterUseCase = new WeightFilterUseCase();

    @Before
    public void init() {
        weights.add(new WeightUI(new Weight(0, 0, 0, "ds", ""), WeightChange.WeightNotChange, 0));
        weights.add(new WeightUI(new Weight(0, 0, 0, "", ""), WeightChange.WeightNotChange, 0));
        weights.add(new WeightUI(new Weight(0, 0, 0, "ds", ""), WeightChange.WeightGrowing, 0));
        weights.add(new WeightUI(new Weight(0, 0, 0, "", ""), WeightChange.WeightDecreases, 0));
    }

    @Test
    public void testFilterOnlyHaveComment() {
        List<WeightUI> filterWeights = weightFilterUseCase.filter(weights, WeightFilterUseCase.WeightFilter.ShowOnlyHaveComment);
        assert filterWeights.get(0).equals(weights.get(0));
        assert filterWeights.get(1).equals(weights.get(2));
        assert filterWeights.size() == 2;
    }

    @Test
    public void testFilterOnlyHaventComment() {
        List<WeightUI> filterWeights = weightFilterUseCase.filter(weights, WeightFilterUseCase.WeightFilter.ShowOnlyHaventComment);
        assert filterWeights.get(0).equals(weights.get(1));
        assert filterWeights.get(1).equals(weights.get(3));
        assert filterWeights.size() == 2;
    }

    @Test
    public void testFilterOnlyWeightGain() {
        List<WeightUI> filterWeights = weightFilterUseCase.filter(weights, WeightFilterUseCase.WeightFilter.ShowOnlyWeightGrowing);
        assert filterWeights.get(0).equals(weights.get(2));
        assert filterWeights.size() == 1;
    }

    @Test
    public void testFilterOnlyWeightDecreases() {
        List<WeightUI> filterWeights = weightFilterUseCase.filter(weights, WeightFilterUseCase.WeightFilter.ShowOnlyWeightDecreases);
        assert filterWeights.get(0).equals(weights.get(3));
        assert filterWeights.size() == 1;
    }
}

WeightChange

public enum WeightChange {
    WeightGrowing(-1),
    WeightDecreases(1),
    WeightNotChange(0);

    int change;

    WeightChange(int change) {
        this.change = change;
    }

    public static WeightChange getByChange(int chane) {
        for (WeightChange weightChange : values()) {
            if (weightChange.change == chane) return weightChange;
        }
        return WeightNotChange;
    }
}

WeightUI

class WeightUI (baseWeight: Weight): Weight(baseWeight){
    var isWeightGrowing:WeightChange = WeightChange.WeightNotChange
    var changeWeight: Double = 0.0
    var isSelect = false

    constructor(weight: Weight,isWeightGrowing : WeightChange,changeWeight:Double) : this(weight){
        this.isWeightGrowing = isWeightGrowing
        this.changeWeight = changeWeight
        this.isSelect = isSelect
    }
}

Weight

@Entity
open class Weight(@PrimaryKey(autoGenerate = true) var id: Long = 0, override var weight: Float = 0.0f,
                  override var date: Long = 0, var comment: String = "",var userName : String = "") : WeightGraphPoint, Parcelable {

    constructor(weight: Weight) : this(weight.id, weight.weight, weight.date, weight.comment,weight.userName)

    constructor(weight: Float,date:Long,userName: String) : this(0,weight,date,userName)

    constructor(parcel: Parcel) : this(
            parcel.readLong(),
            parcel.readFloat(),
            parcel.readLong(),
            parcel.readString() ?: "",
            parcel.readString()?: "")

    override fun writeToParcel(parcel: Parcel, flags: Int) {
        parcel.writeLong(id)
        parcel.writeFloat(weight)
        parcel.writeLong(date)
        parcel.writeString(comment)
        parcel.writeString(userName)
    }

    override fun describeContents(): Int {
        return 0
    }

    companion object CREATOR : Parcelable.Creator<Weight> {
        override fun createFromParcel(parcel: Parcel): Weight {
            return Weight(parcel)
        }

        override fun newArray(size: Int): Array<Weight?> {
            return arrayOfNulls(size)
        }
    }

    override fun equals(other: Any?): Boolean {
        if (this === other) return true
        if (other == null) return false
        val other = other as Weight
        if (weight == other.weight && date == other.date)
            return true
        return false
    }
}

Answer 1

I won't necessarily point out flaws, but also suggestions. Of course you don't have to adhere to any of them, just things to help improve your code. I'm not that familiar with Kotlin, so I am likely to miss some stuff in the Kotlin parts.

WeightFilter

Although it seems like a basic usage of an enum, there is a way to improve the design. Enums in Java are actually a type of class, and so they can have methods. Instead of using a switch to filter by type, we can implement the logic inside:

public enum WeightFilter {
    ShowOnlyHaveComment {
        @Override
        List<WeightUI> filter(List<WeightUI> data) {
            return data.stream().filter(weight -> weight.getComment().trim().isEmpty()).collect(Collectors.toList());
        }
    },
    ShowOnlyHaventComment {
        //implementation
    }
    // other constants
    ;

    abstract List<WeightUI> filter(List<WeightUI> data);
}

public List<WeightUI> filter(List<WeightUI> weights, WeightFilter weightFilter) {
    return weightFilter.filter(weights);
}

There are actual several implementation options, like:

public enum WeightFilter {
    ShowOnlyHaveComment {
        @Override
        Predicate<WeightUI> newFilter() {
            return weight -> weight.getComment().trim().isEmpty();
        }
    },
    ShowOnlyHaventComment {
        //implementation
    }
    // other constants
    ;

    abstract Predicate<WeightUI> newFilter();
}

Why is this design good? The main advantage is that you can modify the enum without having to worry about the switch. It's not uncommon that programmers forget to update all switch over enum which they modified.

WeightChange

We don't really see all the usages of WeightChange, so maybe the can be other comments.

getByChange is an example for a common factory method for Enum by a property. However, the implementation has a small problem.

If the value is not found among the constants, it returns a default value. This is likely pretty bad, since the argument is wrong, but the method acts as if everything is fine. So you might miss bugs that are related to that. Instead, a EnumConstantNotPresentException or IllegalArgumentException should be thrown, indicating the "user" of this method has provided an invalid argument.

WeightFilterUseCaseTest

This is more of a personal approach, so feel free to ignore it.

Tests should generally be completely disconnected from each other, so that they won't have any affect on each other. The sharing of weights and weightFilterUseCase across the instance can lead to an effect among each other, depending on the runner of the tests (which could be changed). I would recommend creating all of those inside each test method as a local variable. It might make the tests longer, but it ensures they are sandboxed. You can use a method to create weights instead of creating each time manually, or switch to JUnit5 and use something like MethodSource.

You might want to become more familiar with assertions from JUnit and matchers from Hamcrest. They both provide more information on test failures, and allow more complex assertions to be done easily.

WeightUI

Not really sure how this is used, since you haven't posted the code for it, however one clear things is that the inheritance of Weight is wrong. Inheritance is used to define type relations, not avoid data duplication, and WeightUI doesn't seem to be a type of Weight.. since Weight is just a container for data about Weight. If anything WeightUI should contain an instance of Weight and use it.

Weight

Again not sure how everything is used here, not the full code.

date can be an actual date object, like Date, instead of long. This can make it easier to use with some things. Really depends on how you use date. Can easily be converted to epoch time for parceling if needed.

Inside equals, after some basic checks, you immediately convert the object to Weight, without testing if it really is an instance of Weight. Could lead to an exception.

Answer 2

I have two considerations ,in your class WeightFilterUseCase you have the following method:

public List<WeightUI> filter(List<WeightUI> weights, WeightFilter weightFilter) {
    switch (weightFilter) {
        case ShowOnlyHaveComment:
            return filterByEmptyComment(weights, true);
        case ShowOnlyHaventComment:
            return filterByEmptyComment(weights, false);
        case ShowOnlyWeightGrowing:
            return filterWeightByChange(weights, WeightChange.WeightGrowing);
        case ShowOnlyWeightDecreases:
            return filterWeightByChange(weights, WeightChange.WeightDecreases);
    }
    throw new EnumConstantNotPresentException(WeightFilter.class, weightFilter.toString());
}

You can rewrite the first two cases of your switch with just one case in a more compact way:

case ShowOnlyHaveComment: case ShowOnlyHaventComment:
    return filterByEmptyComment(weights, weightFilter == WeightFilter.ShowOnlyHaveComment);

Instead of throwing the exception out of the switch you can add the default case and throw there the exception, so your method can be rewritten in this way:

public List<WeightUI> filter(List<WeightUI> weights, WeightFilter weightFilter) {
    switch (weightFilter) {
    case ShowOnlyHaveComment: case ShowOnlyHaventComment:
        return filterByEmptyComment(weights, weightFilter == WeightFilter.ShowOnlyHaveComment);
    case ShowOnlyWeightGrowing:
        return filterWeightByChange(weights, WeightChange.WeightGrowing);
    case ShowOnlyWeightDecreases:
        return filterWeightByChange(weights, WeightChange.WeightDecreases);
    default:
        throw new EnumConstantNotPresentException(WeightFilter.class, weightFilter.toString());
    }
}

I don't know Kotlin, so I cannot help for Kotlin code.