Two functions, or one function with different params?

Question

This is a very generic 'best practice' question, but here's an example.

Let's say I have a movie cataloging app. I want to give my users the chance to specify, say, IMDb or Metacritic for their synopsis/ rating info.

Do I do this:

if (preferredSupplier == "imdb"){
      getIMDbRating(movieName);
}else{
      getMetacriticRating(movieName);
}

Or this:

getRating(movieName, preferredSupplier);

I like the second one better, but it means that function will have to follow vastly different logic depending on the value of the second parameter (for example Metacritic might require a screen scrape, where IMDb might have a nice API).

Or should I combine them? As in getRating() acts as a wrapper function, and calls getIMDbRating() or getMetacriticRating() depending on the value of the second param.

Answer 1

The second one allows you to extend the number of preferred suppliers over time, and you can still (internally) implement these as two seperate methods.

If this were me though, I'd be looking at two classes (Imdb and Metacritic), both derived from a RatingProvider base class, and implementing getRating differently.

Or, if I were getting my Patterns hat on, I'd be looking at the Bridge pattern.

Only you know where the likely change is in your system, and so you know whether you need to go to town on this, but an API where you can getRatings in a uniform way regardless of where they actually came from would, to me, be a better API than one where you have to make those decisions by chosing one method or the other.

Answer 2

I would combine them. As you said, the two may require vastly different implementations and having that implementation code separated into different functions makes things more readable and easier to maintain. But having a single calling point is nice as well. You may consider placing this in a class, marking the implementation functions as private, and the wrapper as public. That way the wrapper is the only point of contact. It would further increase the maintainability by making it obvious that the wrapper is meant to be the single calling point for these functions.

Answer 3

My preference would be for your last idea: use both. Having a function that takes the source as a parameter makes it very easy to write a lot of code that switches from supplier to supplier with a simple value change (perhaps from the UI). You also have the convenient single-supplier functions to call if/when you intend to call with a fixed, known supplier.

Answer 4

The second option is best (b.t.w in OO it is called Factory). Give it a default behavior, and in it call a different function for each provider, No point in all those ifs.

Answer 5

It might be a good idea to have a RatingProvider class with getRating method.

You could have different rating providers as its subclasses, which will have their own implementation of how it would fetch/process the ratings.

Answer 6

I believe it depends on how often you'd use the method(s), from where, and how different the code is. Remember that the main reason to write routines is that they are - that's right - routine. That is, a routine should contain code that you will want to reuse, and should be written in such a way that it is easy to reuse it.

With this said, if I was facing your example case (and was coding in C#) I would probably end up creating an enum for the different types of db searches, then have one wrapper method accepting such an enum as an argument, and from the wrapper method call different data collection method - similar to what you suggested at the end of your post, but not with a string as input.

Answer 7

Wouldn't it sort of depend on how often you are using it? If you are doing that basic check over and over throughout your code, then a function will save you from repeating that code block.

However, if you use that logic only a few times here and there, then it may be clearer to just have it the first way.

Answer 8

Neither solution is ideal. What you should really do is implement preferredSupplier as an interface or an abstract class, with a GetRating(string moviename) function.

Then implement the interface for classes IMDBSupplier and MetaCriticSupplier, and have each class put in it's own logic for getting the rating. This makes the getRating() function totally independent of whatever code is consuming it, which is a good, loosely coupled design.

In your class that is consuming the the suppliers, it doesn't care which type it is - it just calls GetRating(). GetRating() is provider agnostic to the consumer.

Answer 9

If your functions return the same type of response, overloading in a single function can make sense.

If your functions return very different things based upon the inputs, you should probably use two functions, to distinguish that they return different things.

For example if the IMDB rating is a 5 star scale and Metacritic rating were a 4 star scale, you'll want to be clear that they should be handled differently.

Using separate classes would help to clarify, instead of having all these functions out there.

Answer 10

I would define an abstract class called MovieDatabase with an abstract method called getRating, and then provide subclasses that implement the getRating method for various providers (like IMDb, Metacritic).

With this setup, you can write code that is provider-agnostic, i.e. does not know anything about a particular provider. It just expects a MovieDatabase instance and calls the getRating operation on it without worrying about provider or implementation details.

The advantages of this approach is that it's more extensible. If you want to add more providers or operations, you can do so in one place (the MovieDatabase class and its subclasses), and the rest of the code should just work.

Answer 11

Why can't you do something like

public interface IRating{

public Rating getRating(); }

public IMDBRating extends Rating

public MetacriticRating extends Rating