0

So, I'm working on a project right now for school with a few people and one of them has committed some code that I'm really having difficulty wrapping my head around. The basis of the project is creating a music library with songs, albums, and playlists. These playlists, in particular, are arraylists of songs need different ways of sorting and thus he's implemented comparator for sorting. He did so using enums, which I understand from the perspective of just instances to represent items. Like

    public enum Suit {
       SPADES, CLUBS, HEARTS, DIAMONDS
    }

to represent different suits of a card. I also have learned you can declare methods alongside enums, which is what it looks like he did. Here is the attribute declaration area and the constructor:

 public class Playlist implements Comparator<Song>{
    
        private TotalTime aTotalTime;
        private String aName;
        private ArrayList<Song> playList;
        private Comparator<Song> aComparator;
        private enum Method{
            SortByTitle(new Comparator<Song> () {
    
                @Override
                public int compare(Song o1, Song o2) {
                    // TODO Auto-generated method stub
                    return o2.getTitle().compareTo(o1.getTitle());
                    }
                
            }),
            SortByArtist(new Comparator<Song>() {
    
                @Override
                public int compare(Song o1, Song o2) {
                    // TODO Auto-generated method stub
                    return o2.getExpectedTags().getArtist().compareTo(o1.getExpectedTags().getArtist());
                    }
                
            }),
            SortByLength(new Comparator<Song>() {
    
                @Override
                public int compare(Song o1, Song o2) {
                    // TODO Auto-generated method stub
                    return o2.getTotalSeconds()-o1.getTotalSeconds();
                }
                
            });
            private Comparator<Song> comparator;
            Method(Comparator<Song> pComparator){
                comparator = pComparator;   
            }
            public Comparator<Song> getComparator(){
                return this.comparator;
            }
        }
    
        // constructor that initializes the the playlist.
        public Playlist(String pName,Method aMethod) {
            aName = new String(pName);
            playList = new ArrayList<Song>();
            this.aComparator = aMethod.getComparator();
        }
}

I can vaguely follow what's going on here as such: We start with the constructor, which calls aMethod.getComparator(), with aMethod being the enum instance, and then aMethod.getComparator() returns the this.comparator object, which itself is declared three lines above as a private Comparator<Song> comparator. From my perspective, it looks like ithis will return the private comparator object every time and not actually change the sorting method of the Comparable interface. Any help parsing all of this would be greatly appreciated.

Appellus
  • 7
  • 1
  • 1
    You need to also check the place where you actually use the `Method` enum in your code. Above code is all about defining multiple sorting strategies in a smarter way. :) However, what is the exact question or which line you do not understand properly? – Jude Niroshan Sep 19 '20 at 21:11
  • Aside from your question: (1) why `aName = new String(pName);` instead of simple `aName = pName;`? Strings are immutable so what advantage is expected here? (2) why `class Playlist` **`implements Comparator`**?? Should instance of Playlist ever be used like `List songs = ...; songs.sort(playlistInstance)`? – Pshemo Sep 19 '20 at 21:54
  • @JudeNiroshan I guess it's about how providing input to the constructor as one of the defined enums will change the sorting measure. As it looks right now, whatever enum is passed decides down the line how the use of `Collection.sort` down the line will compare the values. But I don't see in the current implementation how it actually changes the `comparator` object. It appears to return the same `private Comparator comparator` each time – Appellus Sep 19 '20 at 21:58
  • @Pshemo 1. I've got no clue why it was done that way 2. The playlist is populated with songs in its `arraylist` attribute, so it was decided we needed a way to organize said `playList`. The playList arraylist will only ever exists inside of the Playlist object. – Appellus Sep 19 '20 at 22:02
  • "*we needed a way to organize said playList*" but that is why you have `private Comparator aComparator;`... so `implements Comparator` is most likely redundant/wrong. – Pshemo Sep 19 '20 at 22:19
  • @Pshemo ah, so we don't need to implement it in `Playlist` if we deal with a direct `comparator` object? Should be implementing our comparator lower down, like in the `Song` class? – Appellus Sep 19 '20 at 22:27
  • Yes, since Playlist *isn't* ordering mechanism but may only *use* it it shouldn't be a subtype of `Comparator` (which is what `implements Comparator` does). But aside from that I am wondering if you even need `private Comparator aComparator;` attribute. Do you think that playlist should have some default ordering? Usually it is user who decides where song should be placed in playlist. Ordering is *optional* functionality (not forced) which may be used occasionally, so knowledge about selected order is necessary only while sorting. – Pshemo Sep 19 '20 at 22:48
  • So it should probably be passed as sorting method's parameter like `void sortSongs(Comparator comparator){ songs.sort(comparator); }` which would let you use it like `playlist.sort(Comparator.comparing(Song::getLength));`. – Pshemo Sep 19 '20 at 22:48
  • Anyway using enum to store some *strategies* is tempting but often incorrect decision since *enums are meant to represent **constants*** (so their number shouldn't change as it could break switch/case which is using it). Since only constant in this world is *change* it is probable that at some point you may needed to add another attribute to Song class like *producer* or *yearOfProduction* and then you will want to sort on it. That would require to add yet another enum value which is what we want to avoid. – Pshemo Sep 19 '20 at 22:51
  • I see, I will bring this to the table and let the rest of my group know about these concerns/new strategies. Thank you! – Appellus Sep 19 '20 at 22:55

2 Answers2

1

Your analysis is correct. This class seems strange. Some points which stand out:

  1. Why is Playlist a Comparator of Songs? It may make more sense to allow the playlist to be sorted using a Method instead of passing on construction.
  2. The Method provided has no impact on the order of Songs in the Playlist.
  3. The Method enum probably should not be private.

It may be worth revisiting the scope of the components in the project.

  1. What is a Playlist? Is it a different Playlist if the Song order has changed?
  2. Should it be up to the Playlist to decide how to play the songs in the playlist?
mike1234569
  • 636
  • 2
  • 5
  • 1.1. I'm not wholly certain, but our topic of study and focus on this project right now is encapsulation, so it could be to limit the client's access to how the Playlist is ordered/internal methods. 1.2. Alright, that's what I thought. What we should be doing instead is returning the `comparator` that correlates to the sorting method/enum. 1.3 I again think it's was done to follow encapsulation principles to prevent direct access 2.1 A playlist is meant to be a collection of `song` objects, which can be ordered in different ways. 2.2 The play method would be from `arraylist.get(0)` to last. – Appellus Sep 19 '20 at 22:08
0

Look only at the enum definition.

The enum definition defines 3 actual enums: SortByTitle, SortByLength, and SortByArtist - those are your SPADE, HEARTS, DIAMONDS, CLUBS; of this enum. For each value, they are initialized with a non-zero-length constructor, and the object passed is a custom impl of a comparator, but forget all that for now.

The enumeration (heh) of enum values then ends. Why? Because semicolon.

But the enum definition doesn't end yet; then we get private Comparator<Song> comparator;.

Note that each individual enum value gets its own copy of this field. Each value of an enum is itself an instance of the 'class' that the enum represents. And the key point here is that this field holds different comparators for SortByArtist, SortByLength, etc.

Therefore, Method.SortByArtist.getComparator(); returns the value of that field for the instance of the Method 'class' (enums are basically classes, with highly limited construction; only one instance per value, so 3 instances here, ever). Which is different from the value of that field for the SortByLength instance.

The rest is just anonymous inner classes.

This is valid java, I think it should be fairly obvious to tell, right?

class StringByLengthComparator implements Comparator<String> {
    public int compare(String a, String b) {
        return a.length() - b.length();
    }
}

...

Comparator<String> c = new StringByLengthComparator();

but we can write that with less characters in java, using the concept 'anonymous inner classes'. This works when you make a class and then intent to use this definition exactly once, and then never use it again. Let's say this 'Comparator c = ...;' line is the only place in the entire code base that you're ever going to mention StringByLengthComparator by name. Then:

Comparator<String> c = new Conmparator<String>() {
    public int compare(String a, String b) {
        return a.length() - b.length();
    }
};

Looks funky, but it means the exact same thing. The one difference is that this class, which still exists, is not named StringByLengthComparator, but it gets a random name you needn't worry about and cannot ever use. That's okay though - after all, this was the only place we were ever gonna use this thing.

Java lambdas.

Note that you can make this even shorter, using lambda syntax:

Comparator<String> c = (a, b) -> a.length() - b.length();

still means the same thing (well, the object you get no longer has presence, which only matters if you do very silly things, such as relying on its object identity hashcode, or lock on the comparator itself, which are all crazy things to do. So if you don't do anything crazy, it's the same thing).

That's what the code is doing. It's no different than first defining 3 classes that each implement Comparable<Song>, and then passing new SongsByLengthComparer() as expression as first argument.

rzwitserloot
  • 85,357
  • 5
  • 51
  • 72
  • Ah, I see. What had me messed up was the enum declarations: `SortByArtist(new Comparator() {}),` I wasn't sure why there was so much stuff inside of the `SortByArtist` parentheses `()`, which are usually reserved for passing arguments. So did he initialize the new `comparator` within `SortByArtist` input parameter? – Appellus Sep 19 '20 at 22:19
  • 2
    It's worth mentioning that when you have an `enum` of comparator strategies, it's also possible to let the `enum` type itself implement `Comparator`, which would be more concise than the delegation to anonymous inner classes (but not as concise as lambda expressions). – Holger Sep 20 '20 at 11:12