Command names are confusing about lastfm HOT 6 CLOSED

I certainly think the method name should match the Api command. Whether it's best to keep each section as TrackApi. etc is a more difficult question.

Either way perhaps there should be an interface or abstract class for each type - I.e. an ITrackApiObject etc - which would allow conditional behaviour to be implemented if necessary.

from lastfm.

Whether it's best to keep each section as TrackApi. etc is a more difficult question.

Yeah I agree, I think all the commands should move to one namespace, separate from the facade objects.

Either way perhaps there should be an interface or abstract class for each type - I.e. an ITrackApiObject etc - which would allow conditional behaviour to be implemented if necessary.

How do you mean?

from lastfm.

Either way perhaps there should be an interface or abstract class for each type - I.e. an >>ITrackApiObject etc - which would allow conditional behaviour to be implemented if necessary.

How do you mean?

If each command inherits from the same interface and same namespace you can't easily tell which command is a Track command and which is an Album command, for example. If they inherit from a base class or interface such as ITrack... and IAlbum... you could tell what time you had by code like:

if (currentObject is ITrack...)
{
//
}

Now, you might not want to be able to do this of course!

from lastfm.

Now, you might not want to be able to do this of course!

IMO anything which checks the type is bad code! It's necessary sometimes but usually only as a last resort... it's a sign you need to rearchitect to make things more abstract with interfaces, generics.

What use case do you see for this? The command types aren't exposed in the public API surface so this would only affect internal usage, and I can't think of anywhere atm that relies on the type of the command used that didn't create the command itself.

If this is really needed I'd prefer extension methods that checked the API method name:

public static bool IsTrackCommand(this LastCommand command)
{
    return command.Method.StartsWith("track");
}

from lastfm.

public static bool IsTrackCommand(this LastCommand command)
{
    return command.Method.StartsWith("track");
}

I would always try to avoid anything with magic strings like this and would use the interface idea instead.. I guess it's a personal choice really!

My version would look like:

public static bool IsTrackCommand(this LastCommand command)
{
    return (command is ITrackCommand);
}

But I can't think of any compelling examples where this is useful. Perhaps something like making fields optional in the parser, but there's likely a better way to do this anyway.

from lastfm.

Yeah that example was a magic string. But magic strings are bad because they make things brittle, and imo the interface thing would be just as brittle. At least that use of the magic string cuts directly to what we want to know - whether the command is a track command (all track commands will have methods that start with "track") - instead of adding another opportunity to mis-code something (forgetting to implement the interface on track commands).

Anyway this is moot because we don't need to do this 😄

I should get to finishing reviewing your PR tonight 👍

from lastfm.