Data sharing problem

[thinhare]

In my program, there are two panels organized in CardLayout, panel 1 for user login, panel 2 for displaying user profile, and a singleton class is created for data sharing between these two panels.

Scenario can be, on panel 1, user type in username and password, click button "login", then in actionPerformed(ActionEvent e), username will be saved in singleton class String username = "somebody" and panel 2 will be made visible on the screen with the username in a JLabel.

My problem is, when I click the button, panel 2 shows up, but the username is lost, seems the change in actionPerformed() does not affect the singleton variable username. But if I put the change String username = "somebody" in constructor, the change takes place.

Any help will be appreciated!

[Deliverance]

Well your Singleton class should only ever be instantiated once, then you can use that instance across many other classes. So assuming you have something like this as your singleton class, you should have no problem accessing the properties of that class:



public class SingletonClass {

private static SingletonClass singleton;
private String userName;

public SingletonClass() {

}
public static SingletonClass getInstance() {
if (singleton == null) {
System.out.println("Returning new server");
singleton = new SingletonClass();
}
return singleton;
}

public String getUserName() {
return userName;
}
public void setUserName(String userName) {
this.userName = userName;
}

}



and have setters / getters for all your other variables as well. Then on both panels, you can do


SingletonClass.getInstance().getUserName();
SingletonClass.getInstance().setUserName("TEST");


etc.

[ProgramThis]

public SingletonClass() {

}

Constructors in a singleton class should always be private. That way you are forcing the use of the static getInstance() method.

[thinhare]

thanks for reply. I've tested different ways of singleton, didn't work out though. the original code was long and a little messy. below is the simplified version which maintains the problem.

This is the main class of the program.


import javax.swing.*;
import java.awt.*;

public class AlbumSys extends JFrame {

JPanel pnlCards = null;
CardLayout layout = null;

public AlbumSys ()
{
layout = new CardLayout();
pnlCards = new JPanel(layout);
pnlCards.add(new PanelAuthen(), "authentication");
pnlCards.add(new PanelAlbumList(), "albumlist");
getContentPane().add(pnlCards);
setDefaultCloseOperation(EXIT_ON_CLOSE);
}

public void init()
{
setLocation(500, 400);
pack();
setVisible(true);

while(true) {
if (Repository.stageChanged) {
switch (Repository.currStage) {
case 0:
layout.show(pnlCards, "authentication");
break;
case 1:
layout.show(pnlCards, "albumlist");
break;
default:
layout.show(pnlCards, "authentication");
}
Repository.stageChanged = false;
}

try {
Thread.sleep(10);
} catch (InterruptedException ie) {
ie.printStackTrace();
}
}
}

public static void main(String[] args)
{
new AlbumSys().init();
}
}



This is the singleton class for data sharing between panels.


public class Repository {

private final static Repository instance = new Repository();
private Repository () {}
public static Repository getInstance() { return instance; }

public static int currStage = 0;
public static Boolean stageChanged = false;
public static String username = "Sad! It's not changed.";
}


This is panel 1 for user login.


import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class PanelAuthen extends JPanel implements ActionListener{

public PanelAuthen()
{
Button btnSignIn = new Button("signin");
btnSignIn.addActionListener(this);
add(btnSignIn);
}

public void actionPerformed(ActionEvent e)
{
String cmd = e.getActionCommand();
if (cmd.equals("signin")) {
Repository.username = "Haha! It's been changed!";
Repository.currStage = 1; // to switch to album list panel
Repository.stageChanged = true;
}
}
}


This is panel 2 for displaying user profile.


import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class PanelAlbumList extends JPanel{

public PanelAlbumList()
{
JLabel lblTitle = new JLabel(Repository.username);
add(lblTitle);
}
}

[ProgramThis]

First:

private final static Repository instance = new Repository();

Change this line to:

private static Repository instance;

And create a new instance of it in the getInstance() method if instance == null (just as Deliverance has already show you how to do...).

Secondly, just because the class is of the Singleton design pattern, and the values are static, that doesn't mean that you shouldn't hold a reference to the object. In order to be able to make that work though, you are going to have to add getter and setter methods (which you should always do anyway instead of directly accessing the data member). This also is something that Deliverance has show for you.

I suggest you take a good look at his version compared to yours and see what you are doing differently. Deliverance took the time to show you how to do it, try the advice.

[thinhare]

hello to ProgramThis,

sorry I didn't make myself clear.

In fact, I tried the way that Deliverance advised first, it did not solve the problem. The reason I put the version that you did not like was because it was shorter and easier for eyes. (you know all setters and getters make the code much longer.)

The version Deliverance advised is put below, the problem stays...

This is the main class of the program.


import javax.swing.*;
import java.awt.*;

public class AlbumSys extends JFrame {

JPanel pnlCards = null;
CardLayout layout = null;

public AlbumSys ()
{
layout = new CardLayout();
pnlCards = new JPanel(layout);
pnlCards.add(new PanelAuthen(), "authentication");
pnlCards.add(new PanelAlbumList(), "albumlist");
getContentPane().add(pnlCards);
setDefaultCloseOperation(EXIT_ON_CLOSE);
}

public void init()
{
setLocation(500, 400);
pack();
setVisible(true);

while(true) {
if (Repository.getInstance().getStageChanged()) {
switch (Repository.getInstance().getCurrStage()) {
case 0:
layout.show(pnlCards, "authentication");
break;
case 1:
layout.show(pnlCards, "albumlist");
break;
default:
layout.show(pnlCards, "authentication");
}
Repository.getInstance().setStageChanged(false);
}

try {
Thread.sleep(10);
} catch (InterruptedException ie) {
ie.printStackTrace();
}
}
}

public static void main(String[] args)
{
new AlbumSys().init();
}
}




This is the singleton class for data sharing between panels.


public class Repository {

private static Repository instance = null;
private Repository () {}
public static Repository getInstance()
{
if (instance == null)
instance = new Repository();

return instance;
}

private static int currStage = 0;
private static Boolean stageChanged = false;
private static String username = "Sad! It's not changed.";

public void setCurrStage(int i)
{
this.currStage = i;
}

public int getCurrStage()
{
return this.currStage;
}

public void setStageChanged(boolean b)
{
this.stageChanged = b;
}

public boolean getStageChanged()
{
return this.stageChanged;
}

public void setUsername(String s)
{
this.username = s;
}

public String getUsername()
{
return this.username;
}
}


This is panel 1 for user login.


import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class PanelAuthen extends JPanel implements ActionListener{

public PanelAuthen()
{
Button btnSignIn = new Button("signin");
btnSignIn.addActionListener(this);
add(btnSignIn);
}

public void actionPerformed(ActionEvent e)
{
String cmd = e.getActionCommand();
if (cmd.equals("signin")) {
Repository.getInstance().setUsername("Haha! It's been changed!");
Repository.getInstance().setCurrStage(1); // to switch to album list panel
Repository.getInstance().setStageChanged(true);
}
}
}



This is panel 2 for displaying user profile.


import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class PanelAlbumList extends JPanel{

public PanelAlbumList()
{
JLabel lblTitle = new JLabel(Repository.getInstance().getUsername());
add(lblTitle);
}
}

[dlorde]

First:

private final static Repository instance = new Repository();

Change this line to:

private static Repository instance;

And create a new instance of it in the getInstance() method if instance == null (just as Deliverance has already show you how to do...).No, that's not needed. Although it seems very common, this 'lazy' initialization isn't necessary in singletons - it's simpler and more robust to do it the original way - because static instances are constructed on first access, not at program load time, so it is equivalent to lazy initialization without the unnecessary extra check every time the instance is required. NALOPKT!

See The Java Language Spec: 12.4.1 When Initialization Occurs (http://java.sun.com/docs/books/jls/second_edition/html/execution.doc.html#44557) (although it's not really a beacon of clarity :rolleyes: ).

Awaken people's curiosity. It is enough to open minds, do not overload them. Put there just a spark...
A. France

[thinhare]

Hello to Dlorde,

I've implemented it in both way, none of them solved the problem. So I suppose the sigleton might not be where the problem lies.

Would you mind taking a look at the below zipped code? thanks.

[dlorde]

From what I can see you're not using Repository as a singleton at all, you're just using it to hold static data. Using static data simply as 'global' variables this way is usually a very bad idea.

Java is an OO language. Objects should collaborate by passing messages between themselves (calling methods), not sharing a common global data pool - unless you're trying to simulate a persistent data store.

For example, if you want to display different text depending on a stage or state indication, you should have a class that keeps track of the stage or state that you can ask if the stage has changed, and if so you can ask it what the new text is. There should be no need for the kind of switch statement you're using.

Good programmers know what's beautiful and bad ones don't...
D. Gelernter

[ProgramThis]

I'm not an expert on the singleton design pattern, but I do believe that you need a reference to the object that is created. Try:

Repository res = Repository.getInstance();

And then using this res variable to set and get information in each of the classes.

[thinhare]

For example, if you want to display different text depending on a stage or state indication, you should have a class that keeps track of the stage or state that you can ask if the stage has changed, and if so you can ask it what the new text is. There should be no need for the kind of switch statement you're using.


to dlorde,

yes, I am more used to procedure-oriented programming, that is why it looks not so OO. but I don't quite get the idea that you said about having a class to keep track of changes, how? could you show me a small example..

[dlorde]

... I do believe that you need a reference to the object that is created. Try:

Repository res = Repository.getInstance();You don't need to use a separate reference, Repository.getInstance() gives you direct access to the Repository instance. The only reason to use a separate reference variable is to make the code look tidier where it is used a lot - and although this is probably a good enough reason, it won't make any difference to the code execution.

And as I suggested before, if the data is static, and the singleton pattern is irrelevant and redundant. By all means provide public accessors ('getters') for the static data, and make it private, but for static data, the singleton is a red-herring.

Oh, and another reason for not doing explicit lazy instantiation in a singleton occurred to me - it's not thread safe, and can't be made thread safe without synchronizing the whole getInstance() method, which is very inefficient as it applies for every call when it's only needed the first time. None of this applies to the combined declaration/instantiation technique.

We flew down weekly to meet with IBM, but they thought the way to measure software was the amount of code we wrote, when really the better the software, the fewer lines of code...
W. Gates

[damien1977]

thinhare,

You need to update the label text in PanelAlbumList to reflect the change in username -

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class PanelAlbumList extends JPanel{
private JLabel lblTitle = new JLabel();

public PanelAlbumList() {
add(lblTitle);
}

public void setTitle(String s) {
lblTitle.setText(s);
}
}

And you need to call the setTitle method before showing the PanelAlbumList. (You'll have to add panelAlbumList as a member of your AlbumSys class).

... snip ...
switch (Repository.currStage) {
case 0:
layout.show(pnlCards, "authentication");
break;
case 1:
panelAlbumList.setTitle(Repository.getInstance().getUserName());
layout.show(pnlCards, "albumlist");
break;
default:
layout.show(pnlCards, "authentication");
}
... snip ...

Not wishing to sound too critical or discouraging, but, I think the major issue here is that the design behind your code is not clear. For example, why would the username be a part of the repository object? I would be inclined to treat the username as part of the authentication process (and probably wrapped up in a User object), and use listeners and events to inform PanelAlbumList of user changes (rather than using the while(true) loop).

For example, here is some rough code to illustrate the idea -

public interface UserUpdateListener {
... snip ...
public void userChanged(UserEvent ue);
... snip ...
}

public class PanelAlbumList extends JPanel implements UserUpdateListener {
public PanelAlbumList() {
... snip ...
AuthenticationManager.getInstance().addUserUpdateListener(this);
}
... snip ...
public void userChanged(UserEvent ue) {
setTitle(ue.getUser().getName());
}
... snip ...
}

public class AuthenticationManager {
... snip ...
public void authenticateUser(User u) {
... snip ...
fireUserChanged(u);
}
... snip ...
// Observer pattern
List<UserUpdateListener> updateListeners = new ArrayList();
public void addUserUpdateListener(UserUpdateListener uul) {
updateListeners.add(uul);
}

protected void fireUserChanged(User newUser) {
for(UserUpdateListener uul : updateListeners)
uul.userChanged(new UserEvent(newUser));
}
}

This may well be over-engineered for what you need. :D

It is tempting with design patterns to look for problems for them to solve (rather than vice versa). I worked with plenty of coders who think everything is a factory one week, then everything is a singleton the next. :D Practise and experience will soon teach you how to use them appropriately.

[thinhare]

Well, I have to admit that it is not really a OO design. and I am working on the redesign, trying to make it a more decent one.

your code's been very explanatory, some parts out of it still need more time for me to digest, though.

I am sure I will come up with new questions.

Thanks.