0

So I read that it is bad design to have an interface parameter be checked as sending in an interface member is supposed to associate itself as an contract that the only the interface members are going to be used. As such I thought to simply overload the method. This seems like it would quickly spiral out of control however if multiple objects that implement the interface needs different implementations of the method.

public IArtist FindArtist(IArtist artist)
        {
            var activeArtist = _database.Artists.FirstOrDefault(a => a.Name == artist.Name);
            return activeArtist;
        }
public IArtist FindArtist(SpotifyArtist artist)
        {
            var spotifyArtists = _database.Artists.Where(a => a is SpotifyArtist).Cast<SpotifyArtist>();
            SpotifyArtist activeArtist = spotifyArtists.FirstOrDefault(a => a.Name == artist.Name && a.SpotifyID == artist.SpotifyID);
            return activeArtist;
        }

In the above code snippet, when I need to call the FindArtist with a SpotifyArtist object (which implements IArtist), the function should look for an object that has the same Name and also SpotifyID. Whereas if it is any other type of IArtist, it is to just return based on the name (probably will modify it to prioritize non-SpotifyArtist objects later). Do you have any suggestions to what I should be doing here instead?

EDIT TableImplementation:

public class MusicObjectTable
    {
        public List<IArtist> Artists;
        public List<IAlbum> Albums;
        public List<ITrack> Tracks;

        public MusicObjectTable()
        {
            this.Artists = new List<IArtist>();
            this.Albums = new List<IAlbum>();
            this.Tracks = new List<ITrack>();
        }

        public MusicObjectTable(List<IArtist> artists, List<IAlbum> albums, List<ITrack> tracks)
        {
            this.Artists = artists;
            this.Albums = albums;
            this.Tracks = tracks;
        }
    }
}
  • 1
    you could use a filter and filter for everything that is set. – horotab Aug 06 '18 at 05:25
  • Sorry I'm not completely sure what you mean. If you mean to check every property, the point of the function isn't to check if every property is equal since the guid generated will be different for every instance of an Artist. Or will it "ignore" the extra SpotifyID for any objects that do not have it? Also by filter I assume you mean the .Where, .Select; etc. LINQ operators? – michael16574 Aug 06 '18 at 05:28
  • Are there only subclasses like spotifyartist or can artists also be stored in the 'main table'? – horotab Aug 06 '18 at 05:31
  • Artists can also be stored in the main table yes. Artist is basically just the SpotifyArtist without the SpotifyID property. Also perhaps future alternatives like "SlackerArtist" might be also be stored in the main table. – michael16574 Aug 06 '18 at 05:34
  • I'm not 100% sure if filtering works then, but if you want me to do so I can give you a code sample you could test. – horotab Aug 06 '18 at 05:36
  • Side note: Why do your methods accept an instance of the return type as an argument? that's almost never a good design... – Zohar Peled Aug 06 '18 at 05:37
  • You can instead supply the `Func` and have the necessary implementation via caller itself – Mrinal Kamboj Aug 06 '18 at 05:37
  • This is my first actual project so I'm coming in without a strong grasp on software engineering principles so I'm learning as I go haha; what should I be doing instead? – michael16574 Aug 06 '18 at 05:39
  • Even a Switch - Case will do well in this case. let me try to provide you some code snippets – Mrinal Kamboj Aug 06 '18 at 05:39
  • Can you show us how your tables look and where you configure your mapping? – horotab Aug 06 '18 at 05:41
  • Wouldn't a switch-case result in having to add onto the switch every time I want to extend? I think that was one of the SOLID principles no goes I read. Though I guess the way the class is right now violates more than a couple of those... @horotab OK editing it, I don't think I have configured mapping? Not sure what you mean by that. – michael16574 Aug 06 '18 at 05:41
  • What type is _database? – horotab Aug 06 '18 at 05:45
  • MusicObjectTable – michael16574 Aug 06 '18 at 05:47
  • I have added some code how you can use filters. – horotab Aug 06 '18 at 06:14
  • Doing a cast operation is a sign of a misfitting design. See TheGeneral's answer to solve your problem. – Spotted Aug 06 '18 at 08:06

3 Answers3

1

OK this is not directly answering your question, because I think it might be a little XY.

However, this is what I'd do, let relational data be relational data. I.e use a relational database.

Artists are a key concept, there is only of them. Make an Artists table.

One Artist may have multiple Spotify accounts, so we might need a Spotify table to hold things like urls, band name, pictures, whatever… I mean one Artist can be in multiple bands right. So the solve here is to have a one-to-many relationship with Artist to Spotify.

You could have the same with YouTube, one artist could have many videos. One-to-Many again.

Every time you need to add more connections (relationships) you just add a new table, you don’t have to expand on the one table (you don't have to keep adding loosely-related junk to the artist table) the only thing you need to add (if you wanted is a navigation collection property).

An example usage is this in a simple pseudo-code way

var artist = db.Include(x => x.SpotifyAccounts)
               .Include(x => x.YouTubeVideos)
               .Include(x => x.EbayStores)
               .FirstOrDefault(x => x.Name == SearchName); 

Console.WriteLine(artist.Name);
if(artist.SpotifyAccounts.Any)
   foreach(var item in artist.SpotifyAccounts)
       Console.WriteLine(" -- " + item.Url);

var spotify = db.SpotifyAccounts
                .Include(x => x.Arists)
                .FirstOrDefault(x => x.Id == SpotifyId);

Console.WriteLine(spotify.Id);
Console.WriteLine(spotify.Url);
Console.WriteLine(spotify.Artist.Name);
              

Note: This does away with your search methods and your inheritance and replaced with relationships. What are the down sides? Well not everything is in the one table, inheritance is really not an option. The pros are, as your models become more complex you just add relationships, you can add as many as you like, which actually doesn't touch your Artist table (unless it's one to one).

When searching for an Artist name, you have access to everything they have. if you search for a particular Spotify account you always have access to the Artist.

However this really depends how far you want to go down the rabbit-hole. If this is going to be any kind of system, I think relational is the way to go. It's scalable, and it's self consistent and it's how most large systems work.

halfer
  • 19,824
  • 17
  • 99
  • 186
TheGeneral
  • 79,002
  • 9
  • 103
  • 141
  • 1
    I really like your reasoning which leads to a much sustainable system. Also kudo for identifying a _potential_ XY problem and proposed an alternative to the dreaded inheritance path. – Spotted Aug 06 '18 at 06:57
  • Wouldn't this design require adding the new tables as properties of the Artist object? Thus requiring the programmer to modify the Artist object directly every time a new table is added? – michael16574 Aug 06 '18 at 20:29
-1

It would be bad form if IArtist behaved differently for different types. It's OK if the implementation is different, and it does different things behind the scenes; it is not OK if the functional behavior differs when viewed as a black box. The contract promises something, and it ought to be consistent.

If you happen to be writing code where you know you have a SpotifyArtist, you also know you can call a different method, if you want to.

public IArtist FindArtistByName(IArtist artist)
{
    var activeArtist = _database.Artists.FirstOrDefault(a => a.Name == artist.Name);
    return activeArtist;
}

public IArtist FindArtistByNameAndID(SpotifyArtist artist)
{
    var spotifyArtists = _database.Artists.Where(a => a is SpotifyArtist).Cast<SpotifyArtist>();
    SpotifyArtist activeArtist = spotifyArtists.FirstOrDefault(a => a.Name == artist.Name && a.SpotifyID == artist.SpotifyID);
    return activeArtist;
}

And you can also provide a convenience method:

public IArtist FindArtistByBestMethod(IArtist artist)
{
    if (artist is SpotifyArtist)
    {
        return repo.FindArtistByNameAndID((SpotifyArtist)artist);
    }
    else
    {
        return repo.FindArtistByName(artist);
    }
}
John Wu
  • 50,556
  • 8
  • 44
  • 80
  • You just brought a convenience method to hide the problem, not resolving the problem itself (no offense intented). – Spotted Aug 06 '18 at 08:19
-1

An answer using filters:

The filter:

public ArtistFilter
{
    public string SearchString { get; set; }
    public Type? Type { get; set; }
    public int? MinimumRating { get; set; }
}

MinimumRating is just to show you how to extend the filter easily.

Secondly you have a method that converts the filter into a function:

private static Expression<Func<IArtist, bool>> CreateArtistFilterExpression(ArtistFilter filter)
{
    Expression<Func<IArtist, bool>> expression = x => true;
    if(filter == null)
    {
        return expression;
    }
    if(!string.IsNullOrWhiteSpace(filter.SearchString))
    {
        expression = expression.And(x => x.Name.Contains(filter.SearchString));
    }
    if(filter.Type != null)
    {
        expression = expression.And(x => x is Type.Value);
    }
    if(filter.MinimumRating != null)
    {
        expression = expression.And(x => x.Rating >= filter.MinimumRating);
    }
    return expression;
}

The And-extension-method is pretty small:

public static Expression<Func<T, bool>> And<T>(this Expression<Func<T, bool>> expr1, Expression<Func<T, bool>> expr2) {
    var invokedExpr = Expression.Invoke(expr2, expr1.Parameters.Cast<Expression>());
    return Expression.Lambda<Func<T, bool>>(Expression.AndAlso(expr1.Body, invokedExpr), expr1.Parameters);
}

A last method reduces redundant code:

public List<IArtist> GetArtistsByFilter(ArtistFilter filter)
{
    var expression = CreateArtistFilterExpression(filter);
    return _database.Artists.Where(expression).ToList();
}

And you can get the best matching one like this:

var filter = new ArtistFilter {
    SearchString = "Lennon",
    Type = typeof(SpotifyArtist)
};
var matchingArtists = GetArtistsByFilter(filter);
var bestMatching = matchingArtists.FirstOrDefault();

You ignore the rating then. By setting the MinimumRating as well you can also filter for only-good artists.

Note: I typed most of this in stackoverflow, so I might have missed a semicolon or so.

horotab
  • 675
  • 3
  • 20
  • Expression trees can be directly consumed in the EF apis, assuming OP is using EF and also helps in getting `Func` at runtime, but here T is unknown and can be varied till resolved, therefore you are relying on properties of `ArtistFilter` ignoring the null ones, which can be a long list for different implementations, making it more complex than simple `switch-case` or `If`. Instead if you can use the `Parameter`, `Member` and `Constant` expression for runtime evaluation and creation of Binary Expression and thus `Func`, it would have been much more flexible implementation – Mrinal Kamboj Aug 06 '18 at 07:13
  • Well I don't use EF but NHibernate. My approach also gives me direct access to the database when I use Fluent. But yes, of course it depends on which framework is used by OP. – horotab Aug 06 '18 at 07:46
  • Interesting approach, however I believe it will confuse the OP since you propose a new feature (as a kind of workaround) instead of trying to solve OP's problem. – Spotted Aug 06 '18 at 08:25
  • That's true, but OP seems to be working on some kind of database for music. If that's true, it might be worth the effort to change to my approach, as the code can be reused in many places. – horotab Aug 06 '18 at 08:38