member field as method parameter in java

Question

Nowadays, I'm writing lots of unit tests in my company.

While writing them, I'm facing a problem regarding usage of member fields as parameters of methods in my test class. That's because my colleague cannot understand of the context of code.

Here's the thing.

public class OrderBOTest{
    private Map<Integer, CartProduct> orderProductMap;
    private Map<Integer, ProductLicenseInfo> licenseInfoMap;
    private OrderSheet orderSheet;

    private User orderedUser;

    @Before
    public void createFixtures() {
        ...

        initializeOrderProductAndLicenseInfoMap();

        ...
    }

    ...

    private void initializeOrderProductAndLicenseInfoMap(){
        orderProductMap = MapUtils.collectionToMap(new ArrayList<CartProduct>(), "productNo");
        licenseInfoMap = MapUtils.collectionToMap(new ArrayList<ProductLicenseInfo>(), "productNo");
    }

    ...

    @Test
    public void shouldAddProductToOrderSheetIfAgeOfUserIsOverThanProductGrade() {
        // Given
        CartProduct product = getCartProduct(PRODUCT_NO);
        setOrderable(product, BUYING_PRODUCT);
        setGradeOfProductAndAgeOfUser(product, gradeCodeTypeOfProduct, ageTypeField, globalAgeOfUser);

        addProduct(product);


        // When
        orderSheet = orderBO.getValidOrderSheet(orderedUser, orderProductMap, agentInfo);          

        // Then
        List<Integer> orderedProducts = new ArrayList<Integer>(orderSheet.getOrderProducts().keySet());
        assertThat(orderedProducts, hasSize(greaterThan(0)));
    }

    private void addProduct(int productNo){
        CartProduct product = createCartProduct(productNo);
        orderProductMap.put(product.getProductNo(), product);
    }

    private void addProductLicenseInfo(int productNo, License license){
        ProductLicenseInfo licenseInfo = new ProductLicenseInfo();
        licenseInfo.setProductNo(productNo);
        licenseInfo.setRightType(license.getCode());

        licenseInfoMap.put(licenseInfo.getProductNo(), licenseInfo);
    }
}

I've extracted orderProductMap and licenseInfoMap as member fields in the class because they are widely used inside the class.

The thing is, my colleague (and even I) cannot figure out that method addProduct(int productNo) adds a CartProduct into orderProductMap.

One piece of advice given to me is to pass orderProductMap into addProduct(int productNo) as a method parameter.

When I heard this advice, I thought it sounded strange because of using member fields as parameters of member method.

Is it common to use member fields as parameters of a member method so that the code's readability is improved?

(added more source code information.)

We don't have enough context to understand if your test is correct, but there is nothing strange here. `bulaBulaTest` is a method of instance and of course it can call any other method on that same instance. — Jean-Baptiste Yunès, Aug 07 '17 at 13:36
I don't see why it would improve readability to add the Map as a parameter to the method. Neither however do I see why it's a Map and not just a List, since the product carries the productnumber with them anyway. — daniu, Aug 07 '17 at 13:40
There is nothing strange about adding information to a parameter instead of adding it to a global variable, and nothing strange about using a field as an argument, if it's unreadable otherwise. — RealSkeptic, Aug 07 '17 at 13:40
It's hard to tell but unless the "add" method is static I'd find passing fields as parameters strange as well. The problem might actually be another one: since those 2 maps seem to provide some sort of infrastructure they might be either part of your application anyways and thus would not be created in the test or they are part of the test's infrastructure and might be put into a separate class. Reusing them between tests wouldn't be wise in most cases. — Thomas, Aug 07 '17 at 13:40
I think that using a class member as a method parameter may confuse even more. You don't sure anymore is it the same object as encapsulated in the object member or not. — Zefick, Aug 07 '17 at 13:40
My only note of caution in all this is to be *certain* you are clearing the contents of your fields (`orderProductMap` and `licenseInfoMap`) in a `@Before` or `@After` method. Unit tests should not depend on being run in a particular order. Actually, here's a second word of advice: if it's unclear that `addProduct` is adding a `CartProduct` to your internal map, **change the method name**. Call it `addNewProductToOrderProductMap` or something. There's no penalty for longer method names, especially in unit testing where readability is essential. — dcsohl, Aug 07 '17 at 14:07
@Jean-BaptisteYunès I'm sorry about that. The test class is regarding ordering products by users and preparing products just before running test is almost always required. So addProduct() method is used so many times. That's why I typed bulaBulaTest. — eminentstar, Aug 07 '17 at 22:01

score 1 · Answer 1 · answered Aug 07 '17 at 13:42

1

One "simple" solution:

private static void addProduct(Map<Integer, CartProduct> orderProductMap, int productNo){ ...

This makes your intention very explicit. You know have a small helper method that helps with code duplication; on the other hand, it is very clear now that the helper updates whatever is given to the method.

And as the method is still private you don't run into "but static is considered bad practice".

And to ask your question (probably opinionated): I think it is bad practice to have methods that have arguments that are fields. The whole point of fields is that you don't need to pass them to internal methods.

Beyond that: consider re-structuring your code if it is that easy to get "lost" while reading testcases.

answered Aug 07 '17 at 13:42

GhostCat

137,827
25
176
248

Or of cource the "complex" solution `private static void add(int no, Function creator, Map addTo) { addTo.add(creator.apply(no)); }` with calls like `add(productNo, this::createProduct, orderProductMap);` and `add(licenceNo, this:createLicenceInfo, licenceInfoMap);`. /s – daniu Aug 07 '17 at 13:51
@daniu I don't follow. I do not see how a three argument solution would be more appealing than a simple `addProduct(this.theMap, 5);` – GhostCat Aug 07 '17 at 13:55
it was kind of a joke (the /s was supposed to meant "end sarcasm"). I find myself playing around with possibilities of "Clean Code"... my pattern avoids duplicate code, but it's hardly more readable. – daniu Aug 07 '17 at 14:11
I see ;-) ... I thought you were explaining why you found my answer interesting, but not worth upvoting. Now I am relieved. Well, wait a minute ... – GhostCat Aug 07 '17 at 14:25

score 1 · Accepted Answer · answered Aug 07 '17 at 13:44

You can just rename your addProduct to addProductToMemberFieldMap (or something more meaningfull) and that will improve readability.

Member fields as parameters seems strange as member methods are already have access to that, so there is no need to them to explicitly provide as parameter.

member field as method parameter in java

2 Answers2