3
\$\begingroup\$

I have a PR for a bugfix in which the fix is just 4 characters. In this situation we are invited to make some refactoring on the class that we are touching if possible.

Venkat Subramaniam's advice is to make lambda expressions just one line, hence I'm trying to move a block of code in a method.

Now choosing a better name come to the scene, and the challenge is to use For+ParamName or not use it on the method name.

With this advice from Google folks (Code Health: IdentifierNamingPostForWorldWideWebBlog):

Omit words that are clear from the surrounding context.

I think that a method name getProducts(Category category) is better than getProductsForCategory(Category category), so could we avoid the For+ParamName?

Before

public class ProductProvider {

private final ProductProviderDatabaseHelper databaseHelper;

public ProductProvider(ProductProviderDatabaseHelper databaseHelper) {
    this.databaseHelper = databaseHelper;
}

public Observable<List<Product>> getProductsForCategory(Category category) {
    if (category == null) {
        return Observable.just(Collections.emptyList());
    }

    return Observable.defer(() -> {
        List<Product> result = new ArrayList<>();

        try {
            // DAO code
            result = daoProduct.query(preparedQuery);
        } catch (SQLException e) {
            Timber.e(e, "Error retrieving Products");
        }

        return Observable.just(result);
    }).subscribeOn(Schedulers.io());
}

After

public class ProductProvider {

private final ProductProviderDatabaseHelper databaseHelper;

public ProductProvider(ProductProviderDatabaseHelper databaseHelper) {
    this.databaseHelper = databaseHelper;
}

public Observable<List<Product>> getProductsForCategory(Category category) {
    if (category == null) {
        return Observable.just(Collections.emptyList());
    }

    return Observable.defer(() -> getProducts(category)
    ).subscribeOn(Schedulers.io());
}
\$\endgroup\$
  • 1
    \$\begingroup\$ while talking about refactoring, in your last return statement I would move the closing bracket before subscribeOn to the line above because it closes the parameters for defer. \$\endgroup\$ – GameDroids Dec 14 '17 at 12:19
  • \$\begingroup\$ indeed I tried to do it but I didn't due to SO editor (maybe I did it in the wrong way) \$\endgroup\$ – Chulo Dec 14 '17 at 12:26
4
\$\begingroup\$

The method name getProducts looks like a getter. However, there is some logic inside and getters usually don't have arguments. getProducts(Category category) seems a bit suspicious for me.

The name getProductsForCategory(Category category) looks too verbose: category is present three times. Why not calling it getProductsFor(Category category)? There would be no repetition and it is read straightforward.

To avoid the confusion with a getter, I also think that it could be named filterProductsBy(Category category) or retrieveProductsFor(Category category).

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I like the filterProductsBy idea, although the getProducts sounds concise to me as well. Methods are overloadable, so getProducts() without parameter could return every product, getProducts(category) would return the products of that category and maybe getProducts(productName) would return any product of that name. \$\endgroup\$ – GameDroids Dec 14 '17 at 12:16

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.