1

Context: Java, fairly new developer

I have inherited code from a friend for a project that processes variables. The first thing i notice is the class has a ton of member variables. I have always been under the impression that a lot of member variables means the class is designed poorly.

The class itself only has one function, to "process" the variables from an input map of varying variable types and with more variables than the class has (it is one step on a multi step variable processing journey) and then return an output map with separate derivation variables. This is currently done by taking in a map type (String, Object), initializing 50 member variables (all related to a the specific function of the class) using the input map, then running helper methods to change the class wide member variables (example later), then using a write method to write the variables to an output map.

I was wondering about the design of this class. Is there a variable processing design structure in Java? Ive heard of Plain Old Data but this is actually doing something with the data and has methods. Still, seeing 50 member variables made my stomach crawl a little. but I dont think I have enough experience to know if it is correct for this functionality or not. It seems the class has one function, it seems like the variables have high cohesion, all the variables are private, the only thing I can see that seems wrong is the helper methods seem to have tight coupling because they use the member variables so if you change any youd have to change it a lot.

I looked up SO and SE questions and many people who said having a lot of member variables usually means low cohesion which i dont think is the case here. I know that global variables are a no no and that passing variables into methods are generally preferred, but I was wondering what the design should be of a class that does processing on a large amount of variables and if there is any best practices. Every example ive seen only deals with like 3-5 variables per class and a couple methods.

public class DataProcessor 
{
    private var1;
    private var2;
    private var3;
    private var4;
    ...
    private var50;


    public Map<String,Object> calculate(Map<String,Object> input) {
        initialize() //wont show but writes each input to the var as the var name same as write
        run()
        write()
        return outputMap
    }
    private run() {
        outputVar = helper();
        outputVar2 = helper2();
        ...
    }
    private helper() {
        var1 = var1 + 10
    }
    private write() {
        writeToMap(outputVar, outputMap)
    }
}
5
  • First things first: those aren't 'class' variables they are 'member' variables. Class variables are preceded with static.
    – JimmyJames
    Jun 9 '20 at 20:18
  • sorry! changed the title to be more clear
    – Ian
    Jun 9 '20 at 20:20
  • 1
    When you get a second, the body needs correction too. Not a huge deal but getting the terminology right will pay off in spades.
    – JimmyJames
    Jun 9 '20 at 20:21
  • changed all to member variables now. Thanks for the pointer.
    – Ian
    Jun 9 '20 at 20:32
  • Sometimes, having a lot of variables is a sign that the wrong data structure is being used. Unless there is in depth usage of the variables, the variables might actually represent data and could be handled more easily and directly with a map and processing of the map contents.
    – Jason K.
    Jun 9 '20 at 23:25
2

It's hard to say for sure because we don't know what var1 through var50 are exactly but in general, yes, this is a code smell. It might mean that the class is trying to do too many things. Another possibility is that this is a 'property bag' a.k.a Data Transfer Object (DTO), a.k.a 'value object'.

The former is pretty widely considered to be a bad idea. See the Single Responsibility Principle (SRP) and the God Object anti-pattern. One word of caution on the SRP: many a developer has wrapped themselves around that axle. It's a good principle but not terribly well-defined.

The latter is something I consider to be typically a sign of an Anemic Domain Model but a lot of people advocate it's use. Personally, I don't see much use in creating hardcoded dictionaries but I'm not going to make too fine of a point on this. There are valid use cases if you treat them like structs. I just think there are usually better options.

The methods you show suggest this is more of the 'too many things' style but it could be a mixture of the two. My suggestion would be to start grouping the variables logically and see if you start seeing some other classes you could break out. You can then make this class a composition of those. Rinse and repeat until you get a decent set of class structures. Then you can go back and refactor out this class.

5
  • yeah that was my first gut reaction. I will look into if i can group them to different objects but generally this code was to make a smaller "group" out of a larger dataset already. For clarification the variables passed in are generally Strings and the helper methods do a lot of string processing on multiple of the input variables to try to check for specific use cases.
    – Ian
    Jun 9 '20 at 21:31
  • and the DTO is like sorta the case. Except the class also does processing on the data as it transfers it between different parts of the overall application.
    – Ian
    Jun 9 '20 at 21:35
  • Is this data isomorphic to a database table i.e. there's a column that aligns to each variable? When I've seen this kind of thing, that tends to be the reason.
    – JimmyJames
    Jun 10 '20 at 13:50
  • Yes the data is grabbed from a bunch of different tables and put together then passed through the system as a big collection then it spits out a processed collection after it goes through all the processors and each datapoint (non processed) is a table column
    – Ian
    Jun 10 '20 at 15:38
  • Are there any repeating patterns? I've seen this kind of thing with highly denormalized data. So if you see things like code_01, code_02, ... code_10 or home_address_1, home_address_2, .... and work_address_1, work_address_2, ... those are opportunities to create a list of items, and types for those items.
    – JimmyJames
    Jun 10 '20 at 16:18
2

You cannot generally say that having fifty variables is bad. Let’s say you are a pilot who has a checklist of 200 items to go through and check. And the test results need to be stored. 200 variables. Of course you can split this into 20 classes of 10 instance variables each, but that doesn’t improve things.

In your case, I dont see any reason for the fifty variables. You have a map with fifty values, each is extracted into an instance variable, each instance variable is replaced with a new value, then each value is written to an output map.

I’d do the simplest thing: Get the first value from the input, calculate the new value, write it to the output. Repeat fifty times. Your decision whether the helper functions read a value, calculate the replacement, write the new value, or just write the new value.

On the other hand, it may be better not to use these dictionaries with fifty elements at all, but use a class with 50 variables everywhere. Not that 50 instance variables is good, but a map containing 50 values is often worse.

1
  • just to be clear, this class grabs 50 variables from a larger collection and does the processing on just the 50 variables. Im not sure since I didnt design but I believe the variables were made because they are different datatypes. This was my first thought though, on why the input is not just processed then written directly. Also not entirely sure why a map is used instead of an object, this is a good question I will ask.
    – Ian
    Jun 9 '20 at 21:22
2

It's unusual for a single object to have 50 variables that wouldn't be better grouped into sub objects.

For example a UK address has 14 fields. You could imagine that an address class that covered addresses in many countries might have 50 fields to cover the different formats. But you could obviously split it into multiple country specific address classes with different fields.

Similarly if I model something complex, for example a cricket match, there will be more than 50 parameters, but I can group them into players, teams, weather etc. objects.

On your question about data processing classes. Yes, there are special classes designed to deal with large datasets where you simply cant hold the entire dataset in memory, but want to write code as if its a simple array. But these tend to deal with large collections of objects rather than single objects with many fields.

for example a DataFrame in python

https://www.geeksforgeeks.org/python-pandas-dataframe/#:~:text=Python%20%7C%20Pandas%20DataFrame,fashion%20in%20rows%20and%20columns.

https://docs.dask.org/en/latest/dataframe.html

2
  • could you point me to this processing class? I looked but must have been searching wrong, I actually have a lot more variables than 50 but just this class takes on the 50 variables. I will see if i can split further too but generally the variables are part of the same overarching "object"/group.
    – Ian
    Jun 9 '20 at 21:28
  • 1
    edited with some links
    – Ewan
    Jun 9 '20 at 21:37
0

From your description it sounds like the variables aren't class members but rather input parameters for the processing step. If this is the case they should be passed in as an argument to the run method. Not as 50 separate arguments of course but as a single container object.

Another thing that comes to mind is the question whether you should go through the trouble of defining a typed variable with a specific meaning for each parameter or not. If those variables can be dealt with as a collection of key-value pairs it could make your life a lot easier. In your processor class, what do you really need to know about these parameters? Can you encode them all in strings?

If you do need the types after all, see Ewan's answer.

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.