62

I have situation, where I want to read configuration file only one time, when class is instantiated.

Suppose I have a method named readConfig(), that reads configuration and puts it into a Map object. When the program is required to use configuration value it reads object with it's define key. I want to know that constructor calls only once it's life cycle. Can I put my method readConfig() into constructor, which would give me benefit of one time calling or is there another mechanism to do that?

trojanfoe
  • 120,358
  • 21
  • 212
  • 242
Sameek Mishra
  • 9,174
  • 31
  • 92
  • 118
  • 2
    Have you looked into the **Singleton Pattern**? – Buhake Sindi Mar 08 '11 at 09:30
  • 1
    this is a very old question.. but if any one wants to use singleton the android way.. have a look at [this](http://stackoverflow.com/q/708012/2219600) – amalBit May 23 '13 at 11:19
  • 3
    Try static block.. the static blocks are run as soon as the first reference is made to rhat class (no matter the object is created or not) and it runs once in the lifetime for that class – Shishir Gupta Sep 17 '14 at 16:28

7 Answers7

79

You can: this is what constructors are for. Also you make it clear that the object is never constructed in an unknown state (without configuration loaded).

You shouldn't: calling instance method in constructor is dangerous because the object is not yet fully initialized (this applies mainly to methods than can be overridden). Also complex processing in constructor is known to have a negative impact on testability.

Connor M
  • 331
  • 3
  • 5
  • 18
Tomasz Nurkiewicz
  • 334,321
  • 69
  • 703
  • 674
28

Better design would be

public static YourObject getMyObject(File configFile){
    //process and create an object configure it and return it
}
jmj
  • 237,923
  • 42
  • 401
  • 438
  • 1
    @sam , if you call method in constructor, some of the member variable's state is corrupted,since object's creation lifecycle is not fully completed. – Dead Programmer Mar 08 '11 at 09:42
  • 3
    I can't work out how that code matches the link to Factory Method Pattern you provided. Common mistake. However, +1 for splitting the reading of the file (could be further factored out into a testable method) and construction of the object that uses the data. – Tom Hawtin - tackline Mar 08 '11 at 12:12
  • @Tom Good Point +1. @sam please read above Tom's comment to make it more testable when you replace `//process and create an object configure it and return it` and Its static factory pattern only Tom, – jmj Mar 08 '11 at 12:16
  • 1
    This doesn't answer the question in the title. It should not be the accepted answer. – amir Sep 13 '19 at 13:40
21

Can I put my method readConfig() into constructor?

Invoking a not overridable method in a constructor is an acceptable approach.
While if the method is only used by the constructor you may wonder if extracting it into a method (even private) is really required.

If you choose to extract some logic done by the constructor into a method, as for any method you have to choose a access modifier that fits to the method requirement but in this specific case it matters further as protecting the method against the overriding of the method has to be done at risk of making the super class constructor inconsistent.

So it should be private if it is used only by the constructor(s) (and instance methods) of the class.
Otherwise it should be both package-private and final if the method is reused inside the package or in the subclasses.

which would give me benefit of one time calling or is there another mechanism to do that ?

You don't have any benefit or drawback to use this way.
I don't encourage to perform much logic in constructors but in some cases it may make sense to init multiple things in a constructor.
For example the copy constructor may perform a lot of things.
Multiple JDK classes illustrate that.
Take for example the HashMap copy constructor that constructs a new HashMap with the same mappings as the specified Map parameter :

public HashMap(Map<? extends K, ? extends V> m) {
    this.loadFactor = DEFAULT_LOAD_FACTOR;
    putMapEntries(m, false);
}

final void putMapEntries(Map<? extends K, ? extends V> m, boolean evict) {
    int s = m.size();
    if (s > 0) {
        if (table == null) { // pre-size
            float ft = ((float)s / loadFactor) + 1.0F;
            int t = ((ft < (float)MAXIMUM_CAPACITY) ?
                     (int)ft : MAXIMUM_CAPACITY);
            if (t > threshold)
                threshold = tableSizeFor(t);
        }
        else if (s > threshold)
            resize();
        for (Map.Entry<? extends K, ? extends V> e : m.entrySet()) {
            K key = e.getKey();
            V value = e.getValue();
            putVal(hash(key), key, value, false, evict);
        }
    }
}

Extracting the logic of the map populating in putMapEntries() is a good thing because it allows :

  • reusing the method in other contexts. For example clone() and putAll() use it too
  • (minor but interesting) giving a meaningful name that conveys the performed logic
davidxxx
  • 125,838
  • 23
  • 214
  • 215
  • This answer deserves to be much higher up the stack as it doesn't just say "don't do it, it *may* cause undefined behavior!", but actually explains *when* (and in turn *why*) it is perfectly safe to put a call to an instance method in the constructor. – Janus Varmarken Nov 08 '18 at 00:36
  • @Janus Varmarken Thank you very much for your feedback. I am glad that you find it helpful. That is not estimated/upvoted as I was very late in my answer against the question date. But well.... – davidxxx Nov 08 '18 at 13:08
2

You can. But by placing this in the constructor you are making your object hard to test.

Instead you should:

  • provide the configuration with a setter
  • have a separate init() method

Dependency injection frameworks give you these options.

public class ConfigurableObject {
   private Map<String, String> configuration;
   public ConfigurableObject() {

   }

   public void setConfiguration(..) {
       //...simply set the configuration
   }
}

An example of the 2nd option (best used when the object is managed by a container):

public class ConfigurableObject {
   private File configFile;
   private Map<String, String> configuration;
   public ConfigurableObject(File configFile) {
       this.configFile = configFile;
   }

   public void init() {
       this.configuration = parseConfig(); // implement
   }
}

This, of course, can be written by just having the constructor

public ConfigurableObject(File configfile) {
    this.configuration = parseConfig(configFile);
}

But then you won't be able to provide mock configurations.

I know the 2nd opttion sounds more verbose and prone to error (if you forget to initialize). And it won't really hurt you that much if you do it in a constructor. But making your code more dependency-injection oriented is generally a good practice.

The 1st option is best - it can be used with both DI framework and with manual DI.

abo
  • 378
  • 4
  • 19
Bozho
  • 588,226
  • 146
  • 1,060
  • 1,140
  • 1
    can you discuss more descriptive form about the second point? – Sameek Mishra Mar 08 '11 at 09:37
  • @sam - instead of doing the work in constructor, invoke the `init()` method from the place where you instantiate the object. But don't do the work in the constructor. See updated – Bozho Mar 08 '11 at 09:38
  • @Tom Hawtin - tackline updated the answer to include an example of the first option (setting the configuration). But public init methods are fine if the object is managed. – Bozho Mar 08 '11 at 12:19
  • In the first example, do you mean calling the setConfiguration() method inside he constructor? – Mistu4u Jan 05 '21 at 03:52
2

The constructor is called only once, so you can safely do what you want, however the disadvantage of calling methods from within the constructor, rather than directly, is that you don't get direct feedback if the method fails. This gets more difficult the more methods you call.

One solution is to provide methods that you can call to query the 'health' of the object once it's been constructed. For example the method isConfigOK() can be used to see if the config read operation was OK.

Another solution is to throw exceptions in the constructor upon failure, but it really depends on how 'fatal' these failures are.

class A
{
    Map <String,String> config = null;
    public A()
    {
        readConfig();
    }

    protected boolean readConfig()
    {
        ...
    }

    public boolean isConfigOK()
    {
        // Check config here
        return true;
    }
};
trojanfoe
  • 120,358
  • 21
  • 212
  • 242
  • 1
    But that makes the error 'more fatal' than you might want it to be; for example you might not mind too much if the call fails, so you wouldn't want to invalidate the whole operation by throwing an exception. – trojanfoe Mar 08 '11 at 12:27
1

Singleton pattern

public class MyClass() {

    private static MyClass instance = null;
    /**
    * Get instance of my class, Singleton
    **/
    public static MyClass getInstance() {
        if(instance == null) {
            instance = new MyClass();
        }
        return instance;
    }
    /**
    * Private constructor
    */
    private MyClass() {
        //This will only be called once, by calling getInstanse() method. 
    }
}
Anurag Tripathi
  • 1,208
  • 1
  • 12
  • 31
Peeter
  • 9,282
  • 5
  • 36
  • 53
  • Singleton could be a solution. But there are better implementations than this one. (It's not thread-safe, and you may prefer to solve it with an Enumeration) – bvdb Apr 22 '14 at 13:48
0

Why not to use Static Initialization Blocks ? Additional details here: Static Initialization Blocks

Community
  • 1
  • 1
Ahmadov
  • 1,567
  • 5
  • 31
  • 48