Clean Code – Is This Way of Calling a Function a Bad Practice?

bad codeclean codecode smelldesigndesign-patterns

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?

Best Answer

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.

Related Solutions

Clean Code – Why is Calling the Next Method from a Previous One Bad Design?

Besides the obvious fact in case #2 createEarth and createPlants do not what their name pretends what they are doing, I think the major flaw here is the violation of the single level of abstraction principle:

createEarth does some work directly, without any abstraction, and then calls createPlants, which is a separate abstraction doing some additional work. But the latter should be on the same level of abstraction as the piece of work done before. So different levels of abstraction are mixed.

Trying to fix this, one would typically first refactor createPlants from #2 like this:

function createPlants(){
    doWork3()  //work 3
    createAnimals()
}

Now is everything on the same abstraction level, so we can resolve the naming issue: doWork3 should better be renamed to createPlants, and createPlants to createLife, since it creates animals and plants:

  function createLife(){
      createPlants()  
      createAnimals()
  }

I think now its obvious that after some further refactoring steps, one automatically ends up in case #1, which follows the SLA principle: inside of createWorld, each call is on the same level of abstraction. Inside each of the other methods, (hopefully) the work done there is also on one level of abstraction.

Note that technically, case #2 will work, too. The problem starts when one has to change something, when the code has to be maintained or evolved. Case #1 creates mostly independent building blocks, which can be easier understood, tested, reused, extended, or reordered on its own. In case #2 each building block depends on another, which throws all the former advantages over board.

C# – Is modifying an object passed by reference a bad practice

The issue here is that a User can actually contain two different things:

A complete User entity, which can be passed to your data store.
The set of data elements required from the caller in order to begin the process of creating a User entity. The system must add a user name and password before it is truly a valid User as in #1 above.

This comprises an undocumented nuance to your object model that isn't expressed in your type system at all. You just need to "know" it as a developer. That aint great, and it leads to weird code patterns like the one you are encountering.

I'd suggest you need two entities, e.g. a User class and an EnrollRequest class. The latter can contain everything you need to know to create a User. It would look like your User class, but without the user name and password. Then you could do this:

public User InsertUser(EnrollRequest request) {
    var userName = GenerateUserName();
    var password = GeneratePassword();

    //You might want to replace this with a factory call, but "new" works here as an example
    var newUser = new User
    (
        request.Name, 
        request.Email, 
        userName, 
        password
    );
    context.Users.Add(user);
    return newUser;
}

The caller starts with only enrollment information and gets back the completed user after it is inserted. This way you avoid mutating any classes and you also have a type-safe distinction between a user who is inserted and one who is not.

Best Answer

Related Solutions

Clean Code – Why is Calling the Next Method from a Previous One Bad Design?

C# – Is modifying an object passed by reference a bad practice

Related Topic