Using Static Methods

[Steve Dyke]

I am trying to use more static class methods in lieu of class constructors.

My detail class:

public class ImportSamplingInspectionDocData {
	
	public static long recseq = 0; 
	
	public static void importSamplingInspectionDocData(ResultSet rs){
		
		try{recseq = rs.getLong("recseq");}catch(Exception e){}
		
	}

}

My list class:

public class ImportSamplingInspectionDocDataList {
	
	public static List<ImportSamplingInspectionDocData> importSamplingInspectionDocDatalist = null;
	
	public static void importSamplingInspectionDocDataList(){
		
		String sql = "";
		List<ImportSamplingInspectionDocData> temp = new LinkedList();
		
		try{
			
			sql = "Select * From webprddt1.wqmszscil Where status = 0";
			
			boolean hasrecords = false;
			Connection dc = QMSGlobalProperties.getDataSource().getAs400conn();
			try{
				Statement stmt = dc.createStatement();
				ResultSet rs = stmt.executeQuery(sql);
					
				
				while(rs.next()){
						
					hasrecords = true;
						
					temp.add(ImportSamplingInspectionDocData.importSamplingInspectionDocData(rs));					
									
				}
				rs.close();
				stmt.close();
			}
			finally{
				dc.close();
			}
			
		}
		catch(Exception e){
			
			System.out.println("ImportSamplingInspectionDocDataList: sql: " + sql);
			System.out.println("ImportSamplingInspectionDocDataList: " + e);
			e.printStackTrace();
			
		}
		
		
	}

}

The line:
temp.add(ImportSamplingInspectionDocData.importSamplingInspectionDocData(rs)); gives the following error message:

The method add(ImportSamplingInspectionDocData) in the type List<ImportSamplingInspectionDocData> is not applicable for the arguments (void)

Do I need to use the class constructor method for this type of usage?

[Paul Clapham]

No, static methods are allowed to return a value. The static method in your detail class doesn't return a value, though. Why not? It doesn't do anything else and your list class clearly expects it to return a value (hence the error message).

[Campbell Ritchie]

Steve Dyke wrote:I am trying to use more static class methods in lieu of class constructors. . . .

Why?

[Steve Dyke]

Campbell Ritchie wrote:

Steve Dyke wrote:I am trying to use more static class methods in lieu of class constructors. . . .

Why?

Because I was told on this forum that static class methods and properties was a more correct way to code than using the new constructor to create an object on a class where nothing changes state.

[Steve Dyke]

Paul Clapham wrote:No, static methods are allowed to return a value. The static method in your detail class doesn't return a value, though. Why not? It doesn't do anything else and your list class clearly expects it to return a value (hence the error message).

I guess I am confused.
I was under the impression that this was setting the value in my detail class.

try{recseq = rs.getLong("recseq");}catch(Exception e){}

[Tim Holloway]

One thing the years have taught me is that almost everytime I set up a static class, sooner or later I have to go back and make it instantianable.

The only real exceptions have been Sith classes, which only deal in absolutes and sometimes not even then, as something as cast-in-stone as a temperature converter class handling Celsius/Fahrenheight may need to be swapped out for one using Kelvin or Rankine.

So my inevitable conclusion is that static-only classes are, as a general rule, a Bad Practice. Be very, very, very sure that you'll never need to create more than one of them. Use a Singleton for preference.

[Steve Dyke]

Tim Holloway wrote:Be very, very, very sure that you'll never need to create more than one of them. Use a Singleton for preference.

Normally my code would be like this that creates an object for each record(see temp.add(new ImportSamplingInspectionDocData(rs));):

sql = "Select * From webprddt1.wqmszscil Where status = 0";
             
            boolean hasrecords = false;
            Connection dc = QMSGlobalProperties.getDataSource().getAs400conn();
            try{
                Statement stmt = dc.createStatement();
                ResultSet rs = stmt.executeQuery(sql);
                     
                 
                while(rs.next()){
                         
                    hasrecords = true;
                         
                    temp.add(new ImportSamplingInspectionDocData(rs));                  
                                     
                }
                rs.close();
                stmt.close();
            }
            finally{
                dc.close();
            }

[Tim Holloway]

Well, if your ImportSamplingInspectionDocData was static within the context of what you're doing, I'd recommend moving the "new operation" out of the loop and using a member function to do the work. Construction isn't a cheap operation.

Also of note: take a look at try-with-resources. Your sample code can explode in several different ways if the database environment isn't what it should be, and try-with-resources can simplify the logic needed to make it better able to adapt to those ways.

[Stephan van Hulst]

Steve Dyke wrote:Because I was told on this forum that static class methods and properties was a more correct way to code than using the new constructor to create an object on a class where nothing changes state.

I think you misinterpreted the advice you were given.

It's true that you can use only static methods if your class is truly nothing more than a namespace for keeping constants and pure functions, but your classes don't fit those criteria. They're full of global variables and stateful operations.

Math.PI is a natural constant, ImportSamplingInspectionDocData.recseq is not. Math.sin() is a pure function, importSamplingInspectionDocData() is not.

Even if your static methods were pure functions, having a lot of them is usually a sign of poor object oriented design. See also this recent topic.

[Tim Holloway]

Stephan van Hulst wrote:
Math.PI is a natural constant, ... Math.sin() is a pure function

... And even they can change if you're into the analysis of possible alternative space-time continuums. We'll assume you're not, but that's what I mean by being very sure!

[Steve Dyke]

Tim Holloway wrote:I'd recommend moving the "new operation" out of the loop and using a member function to do the work. Construction isn't a cheap operation.

Okay, I understand that static classes might not be the best approach.

Can you illustrate your suggestion?

[Stephan van Hulst]

I'd write a class like this:

import java.sql.*;
import java.util.*;

import javax.sql.*;

final class ImportSamplingInspectionDocDataRepository {

  private static final String SELECT_QUERY = "SELECT foo, bar FROM webprddt1.wqmszscil WHERE status = ?";

  private final DataSource dataSource;

  ImportSamplingInspectionDocDataRepository(DataSource dataSource) {
    this.dataSource = dataSource;

    if (dataSource == null)
      throw new IllegalArgumentException("The data source may not be null.");
  }

  List<ImportSamplingInspectionDocData> getAllImportSamplingInspectionDocData() throws RepositoryException {
    try (
      var connection = dataSource.getConnection();
      var statement = connection.prepareStatement(SELECT_QUERY);
    ) {
      statement.setInt(1, 0);
      
      try (
        var resultSet = statement.executeQuery();
      ) {
        var result = new ArrayList<ImportSamplingInspectionDocData>();
        
        while (resultSet.next()) {
          var foo = resultSet.getString("foo");
          var bar = resultSet.getInt("bar");
          result.add(new ImportSamplingInspectionDocData(foo, bar));
        }
        
        return result;
      }
    }
    
    catch (SQLException ex) {
      throw new RepositoryException("An exception occurred while retrieving the import sampling inspection document data.", ex);
    }
  }
}

Obviously, if you don't want to retrieve ALL entries from the database and convert them to objects, then you should add a method that executes a more selective query.

[Steve Dyke]

Stephan van Hulst wrote:I'd write a class like this:

Obviously, if you don't want to retrieve ALL entries from the database and convert them to objects, then you should add a method that executes a more selective query.

With this method is looks like line 34 would still potentially construct many objects.  

[Stephan van Hulst]

Stephan van Hulst wrote:Obviously, if you don't want to retrieve ALL entries from the database and convert them to objects, then you should add a method that executes a more selective query.

An example of a more selective query is one that retrieves a page of entries, instead of all entries.

Worry about getting the right amount of data out of the database, not about converting them to objects.

[Steve Dyke]

Tim Holloway wrote:Also of note: take a look at try-with-resources.

try(Statement stmt = dc.createStatement();){


}

I get: The resource type Statement does not implement java.lang.AutoCloseable
I know that Statement has a close() method.

[Campbell Ritchie]

java.sql/java.sql.Statement does implement AutoCloseable. Please verify that you have picked up that class and not a different kind of Statement. There are several classes with similar names.

[Steve Dyke]

Campbell Ritchie wrote:java.sql/java.sql.Statement does implement AutoCloseable. Please verify that you have picked up that class and not a different kind of Statement. There are several classes with similar names.

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.Statement;

I get the same message on any of these I try to add as a resource.

[Campbell Ritchie]

Have you tried using the fully qualified name of the interface?

try(java.sql.Statement stmt = dc.createStatement();){


}

Please verify that there aren't any other Statement types anywhere.

[Steve Dyke]

Campbell Ritchie wrote:Have you tried using the fully qualified name of the interface?

try(java.sql.Statement stmt = dc.createStatement();){


}

Please verify that there aren't any other Statement types anywhere.

This code gives the same error on the java.sql.Statement part:

try(java.sql.Statement stmt = dc.createStatement()){

[Paul Clapham]

I expect you're using the IBM version of the Java API? Still, try-with-resources and Autocloseable have been in the language since Java 7 and... wait, is that a compiler message or a run-time message?

[Steve Dyke]

Paul Clapham wrote:I expect you're using the IBM version of the Java API? Still, try-with-resources and Autocloseable have been in the language since Java 7 and... wait, is that a compiler message or a run-time message?

Compiler.

I find the Statement class in the rt.jar and it extends the AutoCloseable class.

[Tim Holloway]

Next question is have you checked to see what java version setting you're building with? It would potentially be a problem even using a Java 11 compiler if the compiler had been downshifted to compile for Java 6.

I can't think of how that could result in this particular error, but it's worth checking. IDEs are often with specific Java version compatibility levels in their projects - as for that matter - are batch-built projects such as Maven POMs. So just because you have the latest JDK installed doesn''t guarantee that you're compiling to its full capabilities.

[Mike Simmons]

I think if the compiler were compiling with Java 6 or earlier, the complaint would be that the try-with-resources syntax is not recognized.  If it's looking to see if Statement implements AutoCloseable, then it knows what a try-with-resources is, and that it needs an AutoCloseable.  So I don't think that's the issue.

My own best guess is that, while rt.jar does have the expected Statement.class file in it, there may also be some other jar file in the class path, containing an older Statement definition.  I would look through other libraries being used, especially old libraries that have anything to do with database access.

Steve, are you using an IDE of some sort?  Those are often helpful in finding where the class is declared.  In IntelliJ IDEA, if I do  command-N and type "Statement", I get a pop-up listing various Statement classes I have access to in my classpath, and the jar files they're declared in.  I imagine Eclipse can do something similar.   What IDE are you using, if any?

[Steve Dyke]

Mike Simmons wrote:   What IDE are you using, if any?

Eclipse.

If I delete the java.sql.Statementthisis the list I get for import(I could not figure out how to paste a screen shot).

import 'Statement'(java beans)
import 'Statement'(org.codehaus.groovy.ast.stmt)
import 'Statement'(org.eclipse.jem.java)
import 'Statement'(org.junit.runners.model)
import 'Statement'(java sql)

[Campbell Ritchie]

We don't like screenshots anyway. I am familiar with such dropdown lists on Eclipse and I have clicked the wrong type to import many a time myself. I don't know how to get Eclipse to display the path to an imported class, but you can find a list of dependencies, including XYZ.jar, for each project from project→properties.

[Tim Holloway]

Highlight the word Statement in the try-with-resources and press F4. That should open a Type Hierarchy pane and show the fully-qualified class name.

Actually, you should just be able to hover the mouse pointer over the word Statement and get its autodoc information.

[Steve Dyke]

Tim Holloway wrote:Highlight the word Statement in the try-with-resources and press F4. That should open a Type Hierarchy pane and show the fully-qualified class name.

Actually, you should just be able to hover the mouse pointer over the word Statement and get its autodoc information.

I have two different web apps that I have the java.sql.Statement used in.

App1 lets me use the Statement as a resource. When I do the F4 on the Statement it shows the Type Hierarchy pane(Statement - java.sql).
If I click the Super Type icon it lists the AutoCloseable and Wrapper interfaces.
Also, if I hover over the Statement I do get the autodoc info.
Also, if I highlight Statement and press F3 I get a display of the java.sql.Statement class.

App2(the one in this discussion)
When I do the F4 on the Statement it shows the Type Hierarchy pane(Statement - java.sql).
If I click the Super Type icon it lists the Wrapper interface only.
Also, if I hover over the Statement I do get the autodoc info but only 1 line that says no source JavaDoc could not be found.
Also, if I highlight Statement and press F3 I get a display of the java.sql.Statement Class File Editor.

[Steve Dyke]

Mike Simmons wrote:
My own best guess is that, while rt.jar does have the expected Statement.class file in it, there may also be some other jar file in the class path, containing an older Statement definition.  I would look through other libraries being used, especially old libraries that have anything to do with database access.

I just found a second rt.jar file.
There is one in the JRE System Library
There is one in the Web App Libraries

If I go to the Navigator view, WebContent/Web-INF/lib I find an rt.jar file
If I delete this file all works as expected.

Sorry everyone for this wild goose chase, but thanks for all the help.

[Campbell Ritchie]

It wasn't a wild goose chase at all. Pleased to be able to help

[Tim Holloway]

Ooops.

No, one should never copy jars from the JDK into any Java project. They're already in the JDK, after all! similarly, for webapps, don't include the JEE JARs into the project. In Maven terminology, they are "provided" by the web application server. Although in "incomplete" JEE servers such as Tomcat, that only applies to the core services (JSP and servlet). Stuff like JSF does have to be added to the WAR since they are not in the server (which is why it's "incomplete"/not full-stack).

[Steve Dyke]

Paul Clapham wrote:No, static methods are allowed to return a value. The static method in your detail class doesn't return a value, though. Why not? It doesn't do anything else and your list class clearly expects it to return a value (hence the error message).

What if my detail class has multiple properties?

[Tim Holloway]

Steve Dyke wrote:What if my detail class has multiple properties?

What if, indeed. You can have multiple class (static) properties and/or multiple instance (member) properties in a class. Nothing illegal about that.

[Steve Dyke]

Paul Clapham wrote:No, static methods are allowed to return a value.

I understand that but I am referring to the return value part of your statement.

[Stephan van Hulst]

Return an object that encapsulates the properties.

[Steve Dyke]

Stephan van Hulst wrote:Return an object that encapsulates the properties.

Would this be a candidate for static method(to use as a function)?
Would I return an array, list, ...?

public class SessionMessage {
	
	private String privateusermessage = "";
	private int clrcache = 0;
	
	public SessionMessage(String sessionID){
		
		String sql = "";
		
		try{
			
			sql = "Select msgv7, clrcache From " + AS400LibraryStringsV5.PRODUCTION + ".wqmscuserl" +
			" Where sidv7 = '" + sessionID + "'";
				
			
			try
				(Connection dc = QMSGlobalProperties.getDataSource().getAs400conn();
				Statement stmt = dc.createStatement();
				ResultSet rs = stmt.executeQuery(sql);)
			{
							    		
				if(rs.next()){
				    			
				   privateusermessage = rs.getString("msgv7").trim();
				   clrcache = rs.getInt("clrcache");
				    			
				}
			}
							
		}
		catch(Exception e){
			System.out.println("SessionMessage: sql: " + sql);
			System.out.println("SessionMessage: " + e);
			e.printStackTrace();
		}
		
	}

	public String getPrivateusermessage() {
		return privateusermessage;
	}

	public int getClrcache() {
		return clrcache;
	}

}

[Tim Holloway]

Well, static considerations aside, here's how I would talk to the database:

        try{
             
            final String SQL_COMMAND = "Select msgv7, clrcache+
                           " FROM" + AS400LibraryStringsV5.PRODUCTION + ".wqmscuserl" +
                           " WHERE sidv7 = ?";
            try
                (Connection dc = QMSGlobalProperties.getDataSource().getAs400conn())
            {
                PreparedStatement stmt = dc.PrepareStatement(SQL_COMMAND);
                stmt.setString(1, sessionID);
                ResultSet rs = stmt.executeQuery(sql);)                                       
                if(rs.next()){
                   privateusermessage = rs.getString("msgv7").trim();
                   clrcache = rs.getInt("clrcache");
                }
            }                            
        }
...

Apologies for mis-remembered  method names, syntax, or techniques.

Note that I didn't try-with-resources on the statement or resultset variables. Unless my reasoning is flawed, any Exception thrown in this block will bounce out and automatically close/delete the connection dc, which then should close any and all Statements it owns, which in turn, close their ResultSets. If anyone can say to the contrary, please correct me.

Naturally, if I wanted to handle exceptions on resultset and/or statement operations distinctly, I'd code this a bit differently, but the original code wasn't doing so, and there's no particular gain I can think of to do so anyway. The Java SQL Exception world is rather muddy to begin with.

[Stephan van Hulst]

Steve Dyke wrote:Would this be a candidate for static method(to use as a function)?

I'm not sure what you're asking here. Do you mean that you want to return an instance of SessionMessage from a static method?

If that is the case, you could, but I think it's a bad idea anyway. You should not be able to query the database from a static method. That's similar to using global variables.

Regardless, accessing the database from a constructor is even worse. Constructors should be super simple: just initialize the fields from constructor parameters and keep logic to a minimum. When I'm debugging code of my predecessor and I see a line that creates an object instance using the new keyword, I'm not expecting it to access external systems. If it does, I'm going to break into the author's house and murder them in their sleep.

If you insist on using static methods, you could do something like this:

final class SessionMessage {

  private final String privateUserMessage;

  private final int aMoreDescriptiveNameThanClrCache;

  private SessionsMessage(String privateUserMessage, int aMoreDescriptiveNameThanClrCache) {
    this.privateUserMessage = privateUserMessage;
    this.aMoreDescriptiveNameThanClrCache = aMoreDescriptiveNameThanClrCache;
  }

  static SessionMessage lookupSessionMessage(String sessionId, DataSource pleaseOhPleasePassMeADataSourceInsteadOfMakingMePerformAStaticLookup) throws AppropriateException {
    try ( ... ) {
      // database stuff here

      return new SessionMessage(...);
    }

    catch (SQLException ex) {
      throw new AppropriateException("An exception occurred while lookup up the session message.", ex);
    }
  }
}

But seriously why are you so obsessed with static methods? Stop using them. Use the repository pattern I showed you earlier.

Google for "Service locator anti-pattern" and you will find more than enough articles that explain why performing a static lookup to get your data source is a bad idea.

Another thing: Stop catching exceptions prematurely. Why are you logging exception messages instead of just letting the exception propagate up the call stack until it reaches a call frame that can handle it properly?

[Tim Holloway]

Stephan van Hulst wrote:Regardless, accessing the database from a constructor is even worse.

         

Stephan van Hulst wrote:Constructors should be super simple: just initialize the fields from constructor parameters and keep logic to a minimum. When I'm debugging code of my predecessor and I see a line that creates an object instance using the new keyword, I'm not expecting it to access external systems. If it does, I'm going to break into the author's house and murder them in their sleep.

[Mike Simmons]

Moral: write your code as if Stephan might be maintaining it in the future.  Don't make him come find you.

[Steve Dyke]

Stephan van Hulst wrote:


But seriously why are you so obsessed with static methods? Stop using them. Use the repository pattern I showed you earlier.

I have attempted to use your suggestion.
One concern is the: var connection = dataSource.getConnection(); statemen.
From my understanding a close needs to be performed on the connection in order that the connectionPool can reclaim the connection.