HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Simple expense tracker

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
trackerexpensesimple

Problem

I have posted below a small project of mine in AS3 regarding a super simple Expense Tracker. I finally made it basically functional, and I learnt A LOT in the way. But even I can tell that my design is horrible. If anyone was kind enough as to mention my mistakes to me and/or give me some advice, I would be extremely thankful!

This is basically my main design:

  • DocumentClass calls mainScreen and balanceField.



  • MainScreen calls mainButton.



  • MainButton dispatches Event to mainScreen when clicked to make mainScreen invisible.



  • I repeat the same process with secondaryScreen and secondaryButton (using the same event as trigger).



  • After I click on secondaryButton, InputScreen is called.



  • InputScreen handles the balanceField from within, using static vars from DocumentClass.



  • okButton in inputScreen dispatches Event, same as before to make this invisible and the mainScreen visible again.



Here are some pieces of my code:

DocumentClass:

package
{
    import flash.display.MovieClip;
    import flash.events.Event;

    public class DocumentClass extends MovieClip
    {
        public static var mainScreen:MainScreen;
        public static var balanceField:Balance;
        public static var actual:Number = 0;

        public function DocumentClass() 
        {

            mainScreen = new MainScreen();
            mainScreen.x = 0;
            mainScreen.y = 0;
            addChild(mainScreen);

            balanceField = new Balance();
            balanceField.x = 50;
            balanceField.y = 0;
            addChild(balanceField);            
        }            
    }        
}


This is my main class. Following are 2 subsequent classes:

MainScreen:

```
package {

import flash.display.MovieClip;
import flash.display.SimpleButton;
import flash.events.Event;

public class MainScreen extends MovieClip
{
public var expenseButton:MainButton;
public var incomeButton:

Solution

You said your structure is horrible, but it's not really horrible! I suggest you skim this (a bit old, but relevant) and try to read up on coding patterns (especially AS3 related) so you have a wider knowledge of what is possible.

So, these lines are unused:

this.addEventListener(MouseEvent.ROLL_OVER, onRollOverHandler);
this.addEventListener(MouseEvent.ROLL_OUT, onRollOutHandler);
this.addEventListener(MouseEvent.CLICK, onClickHandler);
this.addEventListener(MouseEvent.MOUSE_DOWN, onPressHandler);


Unless you plan on having features for each of these states, you can take them out. Right after that, you have:

buttonText.text = (categ);


The parentheses around categ aren't necessary. And I'm not sure where buttonText is created, but it looks like it just appears! In the handler below that, the argument myEvent isn't very concise. It's a simple name, which could be more specific, or less possessive (e, evt, handlingEvent, etc.).

Directly below that, you have:

if (category == 'Expense') {
    nextScreen=new SecondaryScreen(category);
}
if (category == 'Income') {
    nextScreen=new SecondaryScreen(category);
}


This could either be turned into a switch(category) if you have plans on changing the action a category makes. However, neither of these categories requires special treatment in these ifs, so why not just take out the conditionals completely! Leave it as a one line assignment.

In the class of InputScreen, a variable wvalue is created and used in this context:

wvalue = inputField.text;
gvalue = Number(wvalue);


Consider removing the extra variable and simplifying it to:

gvalue = Number(inputField.text);


To be consistent, I suggest separating ButtonEvent and Balance into two files. Especially if you decide to add controlling code in the Balance class.
Naming

I touched on this earlier, but it's come back! The names you've chosen for a few things are very misleading and confusing.

First off, DocumentClass really doesn't need Class appended on the end. We know it's going to be a class. However, Document is a bit broad, try finding a name that fits the situation better.

Is public static var balanceField:Balance; an account balance or a field? I think you should change one of those names (variable or class). The Balance class is a bit misleading. I want to believe that a balance class should be handing the business of the deposits and withdraws. Perhaps a better name for that class would be BalanceButton?

In MainButton(categ:String), categ could really just be category. It doesn't hurt.

In InputScreen, the variable gvalue is used. I can't even tell what it stands for! What is the "g"? Same class, the global total is made, but only used in the one function. Unless it's used outside of it, perhaps reduce it's scope as to not pollute your global scope. In fact, reassess the variables that are currently global, and see if they actually do need to be global.

Lastly, I'm not sure what ButtonEvent's DEAD means. Usually events have verbs as constants to use.

Code Snippets

this.addEventListener(MouseEvent.ROLL_OVER, onRollOverHandler);
this.addEventListener(MouseEvent.ROLL_OUT, onRollOutHandler);
this.addEventListener(MouseEvent.CLICK, onClickHandler);
this.addEventListener(MouseEvent.MOUSE_DOWN, onPressHandler);
buttonText.text = (categ);
if (category == 'Expense') {
    nextScreen=new SecondaryScreen(category);
}
if (category == 'Income') {
    nextScreen=new SecondaryScreen(category);
}
wvalue = inputField.text;
gvalue = Number(wvalue);
gvalue = Number(inputField.text);

Context

StackExchange Code Review Q#54089, answer score: 4

Revisions (0)

No revisions yet.