🔒 Locked

Feedback Request

Started by Interesting Dave Apr 10, 2005 at 9:41 PM 5 replies 800+ views

Original Post

Apr 10, 2005 at 9:41 PM

I've recently attempted to teach myself Java ME over the last few weeks and have come up with my first attempt at an application intended to be used on a mobile phone (it actually works on my motorola A1000 :D). Before starting on my next project, I would like to know where I've gone wrong. And so I lay some code before you and ask polietly if you could spare some time to point out the fuckups I've made :) Its just some basic stuff that doesnt really do anything but save 2 user preferences. I hope to be able to expand on it for my next project. Cheers


import javax.microedition.lcdui.*;
import javax.microedition.midlet.*;
import javax.microedition.rms.*;

class Killer
{
    public static App1 app1;
    
    public static void kill(Object obj)
    {
        obj = null;
    }
    
    public static void killApp()
    {
       	app1.killApp();
    }
    
    private Killer()
    {
    }
}

class Globals
{
    public static int ERRORTIMEOUT;
    public static String[] AVALLANGS;
    public static Display display;
    public static Prefs prefs;
    
    private Globals()
    {
    }
}

class LANG
{
	/* This is for different language support */
    public static int LANG;
    public static String LANGUAGE;

	/* Store all string variables here */
    public static String OK;
    public static String BACK;
    public static String CANCEL;
    public static String START;
    public static String OPTIONS;
    public static String EXIT;
    public static String MAINMENU;
    public static String OERRORTIME;
    public static String OLANG;
    public static String ERROR;
    public static String CONFIRM;
    public static String YES;
    public static String NO;

	//errors:
    public static String EFULL;
    public static String EPREFLOAD;
    public static String EPREFSAVE;
    public static String EPREFCLOSE;
	// etc

    public static void setLang(int n)
    {
	if(n == 0)
	{
            LANG = n;
            LANGUAGE = "English";
            OK = "Ok";
            BACK = "Back";
            CANCEL = "Cancel";
            START = "Start";
	    OPTIONS = "Options";
	    EXIT = "Exit";
	    MAINMENU = "Main Menu";
	    OERRORTIME = "Error Timeout";
	    OLANG = "Language";
	    ERROR = "Error!";
	    CONFIRM = "Are you sure?";
	    YES = "Yes";
	    NO = "No";

            EFULL = " Record store full";
            EPREFLOAD = " Error loading preferences";
            EPREFSAVE = " Error saving preferences";
            EPREFCLOSE = " Error closing preferences";
        }
	else if(n == 1)
	{
            LANG = n;
            LANGUAGE = "Français";
            OK = "Continuer";
            BACK = "Précedent";
            CANCEL = "Annuler";
            START = "Commencer";
	    OPTIONS = "Options";
		//etc
	}
    }

    private LANG()
    {
    }
}

class Prefs
{
    RecordStore RS;

    public Prefs()
    {     
	/* Try to load saved preferences, else load defaults */
	try
	{
            RS = RecordStore.openRecordStore("prefs", true);
            if(RS.getNumRecords() == 0)
            {
                loadDefaults();
		RS.addRecord(toString().getBytes(), 0, toString().getBytes().length);
            }
            else
                loadPrefs();
	}
	catch(RecordStoreFullException e)
	{   new ErrorMsg(LANG.EFULL); }
	catch(RecordStoreException e)
	{   new ErrorMsg(LANG.EPREFLOAD); }
    }
	
    public static void loadDefaults()
    {
	Globals.ERRORTIMEOUT = 1500;
	LANG.setLang(0);
    }

    public void loadPrefs()
    {
	try
	{
            String t = new String(RS.getRecord(1));
            String[] temp = new String[2];
            temp[0] = t.substring(0, t.indexOf(":"));
            temp[1] = t.substring(t.lastIndexOf(':')+1);
            Globals.ERRORTIMEOUT = Integer.parseInt(temp[0]);
            LANG.setLang(Integer.parseInt(temp[1]));
	}
	catch(Exception e)
	{ loadDefaults(); new ErrorMsg(LANG.EPREFLOAD); }
    }
	
    public void save()
    {
	try
	{
            RS.setRecord(1, toString().getBytes(), 0, toString().getBytes().length);
	}	
	catch(Exception e)
	{ new ErrorMsg(LANG.EPREFSAVE); }
    }

    public void close()
    {
	try 
	{	
            save();
            RS.closeRecordStore();
	}
	catch(Exception e)
	{ new ErrorMsg(LANG.EPREFCLOSE); }
    }
	
    public String toString()
    {
	return Globals.ERRORTIMEOUT + ":" + LANG.LANG;
    }
}

class ConfirmMsg extends Form implements CommandListener
{
	/* Asks user if they are sure, and accepts a response */

    Displayable display;
    Command yesCmd = new Command(LANG.YES, Command.OK, 1);
    Command noCmd = new Command(LANG.NO, Command.CANCEL, 2);
    CommandListener cmdlisten;
    
    public ConfirmMsg(Displayable d)
    {
        super(LANG.CONFIRM);
        display = d;
        addCommand(yesCmd);
        addCommand(noCmd);
        setCommandListener(this);
        Globals.display.setCurrent(this);
    }
    
    public void addString(String m)
    {
        append(m);
    }
    
    public void setListener(CommandListener cl)
    {
        cmdlisten = cl;
    }
    
    public void commandAction(Command c, Displayable d)
    {
        if(c == yesCmd)
            cmdlisten.commandAction(yesCmd, display);
        else
            Globals.display.setCurrent(display);
        
        Killer.kill(this);
    }
}

class ErrorMsg extends Alert
{
	/* Flashes an error message */

    public ErrorMsg(String m)
    {
	super(LANG.ERROR);
	setString(m);
        setTimeout(Globals.ERRORTIMEOUT);
	Globals.display.setCurrent(this);
    }
        
    public ErrorMsg(String t, String m)
    {
        super(t);
        setString(m);
        setTimeout(Globals.ERRORTIMEOUT);
        Globals.display.setCurrent(this);
    }
	
    public ErrorMsg(String m, Displayable b)
    {
    	super(LANG.ERROR);
    	setString(m);
	setTimeout(Globals.ERRORTIMEOUT);
	Globals.display.setCurrent(this, b);
    }
}
		
class optionsForm extends Form implements CommandListener
{
    Command okCmd = new Command(LANG.OK, Command.SCREEN, 1);
    Command backCmd = new Command(LANG.BACK, Command.SCREEN, 2);
    TextField timeTxt = new TextField(LANG.OERRORTIME, ""+Globals.ERRORTIMEOUT, 9, 0);
    ChoiceGroup langsCg = new ChoiceGroup(LANG.OLANG, 4, Globals.AVALLANGS, null);

    public optionsForm()
    {
    	super(LANG.OPTIONS);

	addCommand(okCmd);
	addCommand(backCmd);
	setCommandListener(this);

	append(timeTxt);
	append(langsCg);
        langsCg.setSelectedIndex(LANG.LANG, true);
    }
	
    public void commandAction(Command c, Displayable d)
    {
	if(c == backCmd)
            Globals.display.setCurrent(new mainList());
	else if(c== okCmd)
	{
            if(timeTxt.getString().equals(""))
		timeTxt.setString("1500");
            if(Integer.parseInt(timeTxt.getString()) > 5000)
                timeTxt.setString("5000");

            Globals.ERRORTIMEOUT = Integer.parseInt(timeTxt.getString());
            LANG.setLang(langsCg.getSelectedIndex());
            Globals.display.setCurrent(new mainList());
            Globals.prefs.save();
        }
    }
}

class mainList extends List implements CommandListener
{
	/* The main menu - only "options" is currently on it */

    Command exitCmd = new Command(LANG.EXIT, Command.SCREEN, 1);    
    
    public mainList()
    {
        super(LANG.MAINMENU, mainList.IMPLICIT, new String[] {LANG.OPTIONS}, null);
        addCommand(exitCmd);
        setCommandListener(this);
    }
        
    public void commandAction(Command c, Displayable d)
    {
    	if(c == exitCmd)
        {
            ConfirmMsg confirm = new ConfirmMsg(this);
            confirm.setListener(this);
        }
	else if(c == mainList.SELECT_COMMAND)
        {
            if(getSelectedIndex() == 0)
            	Globals.display.setCurrent(new optionsForm());
        }
        else
        {
            Killer.kill(this);
            Killer.killApp();
        }
    }
}

public class App1 extends MIDlet
{    
    public App1()
    {
	Prefs.loadDefaults();        
    }
                
    protected void destroyApp(boolean unconditional)
    {
        Globals.prefs.close();
    }

    protected void pauseApp()
    {
    }

    protected void startApp()
    {        
        if(Globals.display == null)
        {
            Globals.display = Display.getDisplay(this);
            Globals.AVALLANGS = new String[] {"English", "Français"};	
            Globals.prefs = new Prefs();
            Globals.display.setCurrent(new mainList());
            Killer.app1 = this;
        }
    }
    
    public void killApp()
    {
        destroyApp(true);
        notifyDestroyed();
    }
}

shmoove

Apr 11, 2005 at 2:49 AM

Your biggest fuckup: way too many classes! For a simple application on a big strong phone like the A1000 this is probably inconsequential, but for real world applications on mass market phones it's a cardinal sin. I'll admit I didn't review the code line by line but as soon as I started going over it it was the first thing I noted. You have a separate class for every tiny bit of functionality in your app. I'd say it's even overkill for a desktop/server Java app (but I'm not an experienced J2SE/J2EE developer so I could be wrong about that). The Killer class is completely superfluous. Having a method in a special class just to set objects to null?

In J2ME development, an important thing to do is to find a healthy middle ground between pure OO and procedural designs. This balance will depend on the application and the devices you plan on targetting.

On the other hand there are some "extra" classes that could server a purpose if they were slightly modified. A class to hold all the constants (strings and numbers), similar to your LANG class could be useful when porting the application between different phones. But in a class like this, you should make all the variables constants (static final), and not put any methods. This way an obfuscator will eliminate the class completely, replacing all the constants just like a #define would in C. There are other ways of handling the difference in constants between phones, but this is the quick and dirty way, and if it works for you that's fine.

shmoove

werekarg

168

werekarg

Apr 11, 2005 at 8:49 AM

i dont know if you're going to code games. but if you do, the localization (LANG) is not handled in that way from commercial point-of-view. a mobile operator will request a version for each language and not a single version that can handle the language setup dynamically.

regarding the class count: indeed, it is not advisable to have many classes. it wont hurt the performance of the application, but it will eat jar size like hell. however, i always went the route of prototyping an j2me application (game) in the OO style, then later proceed with the class merging. it really helps the development process.

shmoove

Apr 11, 2005 at 9:41 AM

It can also hurt performance because of the added indirection of calling methods from a separate class, and the fact that more classes have to be loaded so you are left with less memory to work with (and memory is a very precious resource in our world).

shmoove

Interesting Dave

Author

Interesting Dave

Apr 11, 2005 at 9:41 AM

Thanks for the feedback lads.

Quote:
Original post by shmoove
The Killer class is completely superfluous. Having a method in a special class just to set objects to null?

Yeah, I thought so too, but couldn't think of another way to do:

this = null;

The way I do GUI.. erm.. navigation is to create a new object of the desired new screen and delete (null it so it'll be picked up by the garbage collector) the old one. If I don't set it too null, will it still be collected?
Or shall I change the whole way the navigation works?

Quote:
But in a class like this, you should make all the variables constants (static final), and not put any methods.

If its constant, I wont be able to change it right?
I would like the language setting to be selectable in the options screen.

shmoove

Apr 12, 2005 at 2:48 AM

If an object doesn't have any valid references it will be collected:

Integer i = new Integer(1);i = new Integer(2);

The object holding 1 will be garbage-collected because there are no variables referencing it.

You have to start taking into consideration the constrained resources you'll be dealing with, and sometimes forget OO lessons you learned in J2SE. More examples:
The ConfirmMsg class has this method:

    public void addString(String m)    {        append(m);    }

but append() is a public method in Form, so any place you called the addString() method you might as well have called append() and gotten over with it. You get a smaller application and it runs a little bit faster because there is one less method call.

About the LANG class: yes, you're right. In order to use it like you intend to you can't make them constants, but like werekarg said, MIDlets hardly ever come in multi-language versions. I was talking about making them constants and controlling the language version in the build process, so you can easily make versions for different languages.

shmoove

Zahlman

257

Zahlman

Apr 12, 2005 at 11:09 PM

Another option is to make them "constants" in a data file, and have your code read the data in. Then your build process selects a data file for the appropriate language, and includes that file in the jar.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.