10

I have the following code:

public void moveCameraTo(Location location){
    moveCameraTo(location.getLatitude(), location.getLongitude());
}

public void moveCameraTo(double latitude, double longitude){
    LatLng latLng = new LatLng(latitude, longitude);
    moveCameraTo(latLng);
}

public void moveCameraTo(LatLng latLng){
    GoogleMap googleMap =  getGoogleMap();
    cameraUpdate = CameraUpdateFactory.newLatLngZoom(latLng, INITIAL_MAP_ZOOM_LEVEL);
    googleMap.moveCamera(cameraUpdate);
}

I think that with this way I eliminate the responsibility of knowing what is a LatLng in another class, for example.

And you do not need to prepare the data before calling the function.

What do you think?

Does this approach have a name? Is it really a bad practice?

  • 2
    I think you're just dealing with Encapsulation using the language's built-in Method Overloading. It's not good/bad. Just a tool that can help or hurt your code. – bitsoflogic Feb 21 at 18:11
  • 5
    If your goal is to conceal LatLng from the clients of this Camera class, then you probably don't want moveCameraTo(LatLng) being public. – bitsoflogic Feb 21 at 18:11
  • In addition to the advice given in the answers, this feels like a good place to mention YAGNI. You Ain't Gonna Need It. Don't define API methods before there is a good use case because... You Ain't Gonna Need It. – Patrick Hughes Feb 21 at 23:43
  • Nothing wrong with moveCameraToLocation and moveCameraTo/moveCameraToCoords. Definitely wouldn't want to be passing Location/lat/long all in the same names. – insidesin Feb 22 at 4:56
9

You are using your languages method overloading feature to offer the caller alternate ways of resolving the methods dependency on positional information. You are then delegating to another method to resolve the remaining work of updating the camera.

The code smell here would be if you keep just extending the chain of methods calling methods. The Location taking method calls the double taking method which calls the latLng taking method which finally calls something that knows how to update the camera.

Long chains are only as strong as their weakest link. Each extension of the chain increases the footprint of code that has to work or this thing breaks.

It is a far better design if each method takes the shortest possible path to solving the problem that has been presented to it. That doesn't mean each must know how to update the camera. Each should translate their position parameter type to one uniform type that can be passed to something that does know how to update the camera when presented this one type.

Do it that way and you can remove one without breaking half of everything else.

Consider:

public void moveCameraTo(Location location){
    moveCameraTo( new LatLng(location) );
}

This makes dealing with latitude and longitude LatLngs problem. The cost is it spreads knowledge of LatLng around. That might seem expensive but in my experience it's a preferable alternative to primitive obsession which is what avoiding establishing a parameter object leaves you stuck with.

If Location is refactorable but LatLng is not, consider solving this by adding a factory to Location:

moveCameraTo( location.ToLatLng() );

This also nicely avoids primitive obsession.

  • 1
    Doesn't seem that bad to me -- it's really just a convenience for the consumer. If this were chained 10 times or if the consumer doesn't really need all these options then I'd call it a code smell. – mcknz Feb 21 at 17:20
  • 1
    But what is the alternative put all logic for transform doubles to LagLng or Location to LagLng in each function? – Tlaloc-ES Feb 21 at 17:23
  • @Tlaloc-ES better? – candied_orange Feb 21 at 17:51
  • @Tlaloc-ES The "transform" only needs to happen once when the primitives are being loaded into the domain logic. It'd occur when you deserialize the DTO. Then, all your domain logic gets to pass around LatLng or Location objects. You can show the relationship between the two with New LatLng(Location) or Location.toLatLng() if you need to go from one to the other. – bitsoflogic Feb 21 at 18:17
  • 1
    @Tlaloc-ES Related reading: FlagArgument by Martin Fowler. Read the "Tangled Implementation" section. – Marc.2377 Feb 22 at 2:09
8

There is nothing particularly wrong with your solution.

But my personal preference would be that those methods are not that useful. And just complicate the interface of whatever object they are part off.

The void moveCameraTo(double latitude, double longitude) doesn't really simplify the code, as I see no problem simply calling moveCameraTo(new LatLng(latitude, longitude)); in it's place. This method also smells of primitive obsession.

The void moveCameraTo(Location location) could be better solved by proving Location.ToLatLng() method and calling moveCameraTo(location.ToLatLng()).

if this was C# and if such a methods were truly necessary, I would prefer them as extension methods instead of instance methods. Usage of extension methods would become really obvious if you tried abstract away and unit-test this instance. As it would be much easier to just fake out single method instead of multiple overloads with simple conversions.

I think that with this way I eliminate the responsibility of knowing what is a LatLng in another class, for example.

I see no reason why this would be a problem. As long as your code references class that contains void moveCameraTo(LatLng latLng), it still indirectly depends on LatLng. Even if that class is never directly instantiated.

And you do not need to prepare the data before calling the function.

I don't understand what you mean. If it means creating new instance or transforming classes from one into another, I see no problem with that.

Thinking about it, I feel that what I'm saying is also supported by API design of .NET itself. Historically, lots of .NET classes followed your approach of having lots of overloads with different parameters, and simple conversions inside. But that was before extension methods existed. More modern .NET classes are more light-weight in their own APIs and if there are any methods with parameter overloads, they are provided as extension methods. Older example is NLog ILogger that has dozens of overloads for writing to log. Compare that to newer Microsoft.Extensions.Logging.ILogger that has total of 3 methods (and only 1 if you count logging itself). But there are lots of helpers and various parametrizations as extension methods.

I think this answer shows that some languages would have tools to make design like this nicer. I don't know much Java, so I'm not sure if there would be any equivalent. But even using plain static methods might be an option.

  • Not sure why but I find myself rooting for this answer despite having a competing one. I think you just managed to make the point better. My only feedback would be to remember that not everyone dealing with this issue is on .NET. +1 – candied_orange Feb 21 at 19:00
  • @candied_orange Right. Now that I look better at OP's question, that looks more like Java than C#. – Euphoric Feb 21 at 19:02
  • I think that really have not a problem in use moveCameraTo(new LatLng(latitude, longitude)); in anyplace of the project, but i think that is more clear use directly the moveCametaTo(latLng), is like a File in java you can pass path like string, or like a Path class – Tlaloc-ES Feb 21 at 22:41
1

I'm not sure what the proper name for this is, if it has one, but it is good practice. You expose multiple overloads, allowing the calling class to determine which parameter set it wants to use. As you say, another class may not know what a LatLng object is, but might know its Location.

Having one method call the other is key too, as you don't want duplicated code across those methods. As you've done, pick one method to be the one that does the work, and have the other methods call it (directly or indirectly)

1

If you mind using kind of methods, it is just method-overloading feature it is good.

But there is not eliminate the responsibility of knowing what is a LatLng. Because you are initializing LatLng latLng = new LatLng(latitude, longitude). This is totatly dependency to LatLng. (To understand why Initializing is a dependency problem, you can check Dependency Injection) Creating overloaded method just helps clients who don't care LatLng. If you mean this, it is also good but I dont think it is an approach. It is just many service methods for clients.

So, there are two options to design your architecture:

  1. Create many overloaded method and provide them to client.
  2. Create few overloaded methods which need parameter(s) as interface or concrete class.

I run away as possible as from creating methods which need primitive types as parameters(Option 1). Because if your business changes many time and you need to play method parameters, it is really hard to change and implement all caller function.

Instead of this, use interfaces(Dependency Injection). If you think it is cost and takes more time, then use classes and provide their mapper extension methods(Option 2).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.