5

First example:

countryRepository.getCountriesFromAsiaWhereAreTheMostPlanesAndBoats();
countryRepository.getCountriesFromAfricaWhereAreTheMostCars();

Are these names too big? How else do we call these functions?

I was thinking about parameters, but according to Martin Fowler, it's better not to use parameters in functions.

Second example:

I have to use these two variables in the class:

public static class CompareCountries {     
    public String[] countriesFromAsiaWhereAreTheMostPlanesAndBoats;
    public String[] countriesFromAfricaWhereAreTheMostCars;

    public CompareCountries (String countriesFromAsiaWhereAreTheMostPlanesAndBoats, String[] countriesFromAfricaWhereAreTheMostCars){
        this.countriesFromAsiaWhereAreTheMostPlanesAndBoats = countriesFromAsiaWhereAreTheMostPlanesAndBoats;
        this.countriesFromAfricaWhereAreTheMostCars = countriesFromAfricaWhereAreTheMostCars;
    }   

    public static String[] compares() {}
}

Here the more I can not use parameters.

  • 18
    Use parameters. Martin Fowler is not God, and this is one of his stupider rules. Be pragmatic and sensible: following any "rule" blindly leads to bad code. – user949300 Mar 3 '18 at 16:34
  • 2
    "but according to Martin Fowler, it's better not to use parameters in functions" I have to agree with @user949300, but you probably took these examples too far from context and misunderstood the advice (rule). – πάντα ῥεῖ Mar 3 '18 at 16:51
  • 1
    Is anyone else perplexed by ... Countries ... WhereAreTheMost ...? To me this is missing a threshold (either % or ordinal) – Caleth Mar 6 '18 at 14:55
  • 1
    This seems like the kind of query that would be easier in SQL. SELECT * FROM Countries WHERE Continent = 'Asia' ORDER BY NumPlanes + NumBoats DESC LIMIT 1. – dan04 Mar 6 '18 at 22:49
  • 2
    I don't agree that this 'rule' from Martin Fowler is stupid. It's misunderstood. A method without any parameter is easier to use, easier to test and shows cohesiveness. In this case it would be more logical to have a addContinent(continent) method, and then a compare() method. The method to setup the class wouldn't even have to be tested as it would by default be tested through testing compare(). And the class would scale a lot more. – Steve Chamaillard Mar 7 '18 at 7:04
11

Never let a code layout style issue motivate you to shorten a name:

public static class CompareCountries {     
    public String[] countriesFromAsiaWhereAreTheMostPlanesAndBoats;
    public String[] countriesFromAfricaWhereAreTheMostCars;

    public CompareCountries (
            String countriesFromAsiaWhereAreTheMostPlanesAndBoats, 
            String[] countriesFromAfricaWhereAreTheMostCars
    ) {
        this.countriesFromAsiaWhereAreTheMostPlanesAndBoats =
            countriesFromAsiaWhereAreTheMostPlanesAndBoats
        ;

        this.countriesFromAfricaWhereAreTheMostCars = 
            countriesFromAfricaWhereAreTheMostCars
        ;
    }   

    public static String[] compares() {}
}

However, always try to get the point quickly and effectively. Never permit pointless fluff.

Consider:

asianContriesWithMostPlanesAndBoats
africanCountriesWithMostCars

Also, I find it surprising that a class whose responsibility is to "compare countries" only deals with Asia and Africa. The class name might need to be longer. I shouldn't be surprised when I look inside.

  • 4
    And when this scales to cover all 7 continents and Planes, Trains, Cars, Boats, Buses and Subways, that will all work out fine, right? :-) – user949300 Mar 4 '18 at 1:19
  • 2
    @user949300 too right. This example is begging for a redesign. – candied_orange Mar 4 '18 at 9:26
10

If you really don't want to use parameters, use divide and conquer, a.k.a. SRP. Split your one long call into several, e.g.

getCountries()
  .fromAsia()
  .planesAndBoats()

This style is often called a "fluent interface".

  • 4
    Not that I disagree in principle just that this particular proposal is ambiguous. getCountries... clear enought... from asia... a filter by continent... planes and boats... another filter ? if so, countries from asia that have planes AND boats, the most of both? or does it return a list of all planes and boats from all countries in all asia ?... naming is important and when changing naming scheme one has to be sure to not remove information from the original meaning – Newtopian Mar 6 '18 at 14:33
  • appart from this nitpicking fluent style can provide alternatives to manage the kind of complexities exposed by the OP – Newtopian Mar 6 '18 at 14:34
  • @Newtopian I agree the names are a sketchy, but I'm more or less following OP. Thanks for mentioning "fluent style", will add to my answer. – user949300 Mar 7 '18 at 6:22
2

Personally, I'm ok with long variable names, but if it was me, I'd probably name countriesFromAsiaWhereAreTheMostPlanesAndBoats something like topCountriesByAirSea.

However, having said that, if the constructor is recieving two lists of countries, then what those lists contain is not relevant to this class! And if it is relevant, it's probably a design flaw.

If this is the case, I would probably name them something like sourceCountries and targetCountries.

1

Long variable names are fine, but in your example it seems to be due to a larger problem.

What about if South America, Europe, and the Pacific are added? Will there be new methods for each of these? What if even more types of vehicles are added? For each country you'd need a new method. A better way to go about it would be to provide a way to filter your data on the repository instead of creating a new method for every specific instance.

In C# you could use Linq. I'm unfamiliar with Java, but perhaps you could create extensions such as these. In Python you could use list comprehensions.

Could sub in a variable which constituents "most" for cars/boats/planes, for now 100:

countryRepository.getCountries()
   .Where(c => c.Region == "Asia")
   .Where(c => c.NumberPlanes > 100 && c.NumberBoats > 100)
   .Select(c => c.Name);

countryRepository.getCountries()
   .Where(c => c.Region == "Africa")
   .Where(c => c.NumberCars > 100)
   .Select(c => c.Name);

Or take the top 10

countryRepository.getCountries()
   .Where(c => c.Region == "Africa")
   .OrderByDescending(c => c.NumberCars)
   .Take(10)
   .Select(c => c.Name);
0

You can bypass the long variable names adding more meaning in the name of a new class.

In the first example, you can create different Repositories: AsiaCountryRepository and AfricaCountryRepository:

asiaCountryRepository.getWhereAreTheMostPlanesAndBoats();
africaCountryRepository.getWhereAreTheMostCars();

In the second example, instead of use only a CompareCountries class, create a CompareAsiaCountries and CompareAfricaCountries:

public static class CompareCountries {     
    public CompareAsiaCountries whereAreTheMostPlanesAndBoats;
    public CompareAfricaCountries whereAreTheMostCars;

    public CompareCountries (CompareAsiaCountries whereAreTheMostPlanesAndBoats, CompareAfricaCountries whereAreTheMostCars){
        this.whereAreTheMostPlanesAndBoats= whereAreTheMostPlanesAndBoats;
        this.whereAreTheMostCars= whereAreTheMostCars;
    }   

}

I know that could be not applicable for all the cases and need some refactor, but sometimes this is a good solution.

More one example

You can also use the package name to add more information about the real responsibilities of the class and variable inside.

Imagine that you calculating a credit from a person that is a foreigner from Asia. Instead of have something like:

import com.application;
com.application.CalculationService {

    public BigDecimal calculate(){
        BigDecimal valueCeditOfForeignerFromAsia = result()
    }

}

You can have a class with a meaningful name (like my last example):

import com.application;
CalculationCreditForeignAsiaService {

    public BigDecimal calculate(){
        BigDecimal value = result();
    }

}

Or you can have a class in a nice package with a lot of meaning:

import com.application.credit.foreigner.asia;
CalculateService() {
    public BigDecimal calculate(){
        BigDecimal value = result()
    }
}

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.