26
\$\begingroup\$

I have written some code to track buffs as a side addition to the popular game League of Legends.

My code is incredibly repetitive and I also have the issue of not being able to track multiple buffs, although that might not be on topic for the question so feel free to not consider this when answering.

import java.util.Scanner;


public class MainProgram {

    public static void main(String[] args) {

        Scanner keyboard = new Scanner(System.in);    

        System.out.println("Jungle Timers v1.0");

        System.out.println("\nSelect a buff to time:");
        System.out.println("\n1. Blue");
        System.out.println("2. Enemy Blue");
        System.out.println("3. Red");
        System.out.println("4. Enemy Red");
        System.out.println("5. Dragon");
        System.out.println("6. Baron");

        System.out.print("\n>  ");
        int timerChoice = keyboard.nextInt();

        keyboard.close();

        switch (timerChoice) {
        case 1:
            friendlyBlue();
        case 2:
            enemyBlue();
        case 3:
            friendlyRed();
        case 4:
            enemyRed();
        case 5:
            Dragon();
        case 6:
            Baron();
            }

    }

    public static void friendlyBlue() {
        System.out.println("\nTracking your blue...");
        System.out.println("5 Minutes left");
        int friendlyBlue = 300;
        long startTime = System.currentTimeMillis() / 1000;
        long endTime = startTime + friendlyBlue;

        while (System.currentTimeMillis() / 1000 < endTime) {
            while (startTime != System.currentTimeMillis() / 1000) {
                startTime += 1;
                if (endTime - startTime > 1)
                    if (endTime - startTime == 240)
                        System.out.println("4 Minutes left");
                    if (endTime - startTime == 180)
                        System.out.println("3 Minutes left");
                    if (endTime - startTime == 120)
                        System.out.println("2 Minutes left");
                    if (endTime - startTime == 60)
                        System.out.println("1 Minute left");
                    if (endTime - startTime == 30)
                        System.out.println("30 Seconds left");
                    if (endTime - startTime == 15)
                        System.out.println("15 Seconds left");
                    if (endTime - startTime == 5)
                        System.out.println("5 Seconds left");
                else if (endTime - startTime == 1) {
                    System.out.println("Blue Buff is up!");
                }
            }
        }
    }

    public static void enemyBlue() {
        System.out.println("\nTracking enemy blue...");
        System.out.println("5 Minutes left");
        int enemyBlue = 300;
        long startTime = System.currentTimeMillis() / 1000;
        long endTime = startTime + enemyBlue;

        while (System.currentTimeMillis() / 1000 < endTime) {
            while (startTime != System.currentTimeMillis() / 1000) {
                startTime += 1;
                if (endTime - startTime > 1)
                    if (endTime - startTime == 240)
                        System.out.println("4 Minutes left");
                    if (endTime - startTime == 180)
                        System.out.println("3 Minutes left");
                    if (endTime - startTime == 120)
                        System.out.println("2 Minutes left");
                    if (endTime - startTime == 60)
                        System.out.println("1 Minute left");
                    if (endTime - startTime == 30)
                        System.out.println("30 Seconds left");
                    if (endTime - startTime == 15)
                        System.out.println("15 Seconds left");
                    if (endTime - startTime == 5)
                        System.out.println("5 Seconds left");
                else if (endTime - startTime == 1) {
                    System.out.println("Enemy Blue Buff is up!");
                }
            }
        }
    }

    public static void friendlyRed() {
        System.out.println("\nTracking your red...");
        System.out.println("5 Minutes left");
        int friendlyRed = 300;
        long startTime = System.currentTimeMillis() / 1000;
        long endTime = startTime + friendlyRed;

        while (System.currentTimeMillis() / 1000 < endTime) {
            while (startTime != System.currentTimeMillis() / 1000) {
                startTime += 1;
                if (endTime - startTime > 1)
                    if (endTime - startTime == 240)
                        System.out.println("4 Minutes left");
                    if (endTime - startTime == 180)
                        System.out.println("3 Minutes left");
                    if (endTime - startTime == 120)
                        System.out.println("2 Minutes left");
                    if (endTime - startTime == 60)
                        System.out.println("1 Minute left");
                    if (endTime - startTime == 30)
                        System.out.println("30 Seconds left");
                    if (endTime - startTime == 15)
                        System.out.println("15 Seconds left");
                    if (endTime - startTime == 5)
                        System.out.println("5 Seconds left");
                else if (endTime - startTime == 1) {
                    System.out.println("Red Buff is up!");
                }
            }
        }
    }

    public static void enemyRed() {
        System.out.println("\nTracking enemy red...");
        System.out.println("5 Minutes left");
        int enemyRed = 300;
        long startTime = System.currentTimeMillis() / 1000;
        long endTime = startTime + enemyRed;

        while (System.currentTimeMillis() / 1000 < endTime) {
            while (startTime != System.currentTimeMillis() / 1000) {
                startTime += 1;
                if (endTime - startTime > 1)
                    if (endTime - startTime == 240)
                        System.out.println("4 Minutes left");
                    if (endTime - startTime == 180)
                        System.out.println("3 Minutes left");
                    if (endTime - startTime == 120)
                        System.out.println("2 Minutes left");
                    if (endTime - startTime == 60)
                        System.out.println("1 Minute left");
                    if (endTime - startTime == 30)
                        System.out.println("30 Seconds left");
                    if (endTime - startTime == 15)
                        System.out.println("15 Seconds left");
                    if (endTime - startTime == 5)
                        System.out.println("5 Seconds left");
                else if (endTime - startTime == 1) {
                    System.out.println("Enemy Red Buff is up!");
                }
            }
        }
    }

    public static void Dragon() {
        System.out.println("\nTracking dragon...");
        System.out.println("6 Minutes left");
        int dragon = 360;
        long startTime = System.currentTimeMillis() / 1000;
        long endTime = startTime + dragon;

        while (System.currentTimeMillis() / 1000 < endTime) {
            while (startTime != System.currentTimeMillis() / 1000) {
                startTime += 1;
                if (endTime - startTime > 1)
                    if (endTime - startTime == 300)
                        System.out.println("5 Minutes left");
                    if (endTime - startTime == 240)
                        System.out.println("4 Minutes left");
                    if (endTime - startTime == 180)
                        System.out.println("3 Minutes left");
                    if (endTime - startTime == 120)
                        System.out.println("2 Minutes left");
                    if (endTime - startTime == 60)
                        System.out.println("1 Minute left");
                    if (endTime - startTime == 30)
                        System.out.println("30 Seconds left");
                    if (endTime - startTime == 15)
                        System.out.println("15 Seconds left");
                    if (endTime - startTime == 5)
                        System.out.println("5 Seconds left");
                else if (endTime - startTime == 1) {
                    System.out.println("Dragon is up!");
                }
            }
        }
    }

    public static void Baron() {
        System.out.println("\nTracking baron...");
        System.out.println("7 Minutes left");
        int baron = 420;
        long startTime = System.currentTimeMillis() / 1000;
        long endTime = startTime + baron;

        while (System.currentTimeMillis() / 1000 < endTime) {
            while (startTime != System.currentTimeMillis() / 1000) {
                startTime += 1;
                if (endTime - startTime > 1)
                    if (endTime - startTime == 360)
                        System.out.println("6 Minutes left");
                    if (endTime - startTime == 300)
                        System.out.println("5 Minutes left");
                    if (endTime - startTime == 240)
                        System.out.println("4 Minutes left");
                    if (endTime - startTime == 180)
                        System.out.println("3 Minutes left");
                    if (endTime - startTime == 120)
                        System.out.println("2 Minutes left");
                    if (endTime - startTime == 60)
                        System.out.println("1 Minute left");
                    if (endTime - startTime == 30)
                        System.out.println("30 Seconds left");
                    if (endTime - startTime == 15)
                        System.out.println("15 Seconds left");
                    if (endTime - startTime == 5)
                        System.out.println("5 Seconds left");
                else if (endTime - startTime == 1) {
                    System.out.println("Baron is up!");
                }
            }
        }
    }
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ You do know, something like that already exists? blog.overwolf.com/releases/overwolf-version-0-33-199-released \$\endgroup\$
    – Vogel612
    Commented Apr 24, 2014 at 14:45
  • 16
    \$\begingroup\$ @Vogel612 I do, but this seemed interesting to me for learning purposes :-) \$\endgroup\$
    – Niall Szalkai
    Commented Apr 24, 2014 at 14:56
  • 4
    \$\begingroup\$ @Vogel612 Many things actually. Including countless apps. That being said, hello world exists 100584902 times, and someone new codes it every day. OP surely learned something valuable about methods/functions here. \$\endgroup\$
    – Cruncher
    Commented Apr 24, 2014 at 20:12

9 Answers 9

Reset to default
21
\$\begingroup\$

(relatively critical review ... apologies in advance)

The two issues I feel are most incorrect about your code is the buggy switch statement, and the poor choice of timing mechanism. The decision to use these mechanisms has lead to a poor OOP design.

Switch

First, though, the switch bug:

switch (timerChoice) {
case 1:
    friendlyBlue();
case 2:
    enemyBlue();
case 3:
    friendlyRed();
case 4:
    enemyRed();
case 5:
    Dragon();
case 6:
    Baron();
    }

If the user chooses 1, they will do everything!!! Your switch cases should 'break':

switch (timerChoice) {
case 1:
    friendlyBlue();
    break;
case 2:
    enemyBlue();
    break;
case 3:
    friendlyRed();
    break;
case 4:
    enemyRed();
    break;
case 5:
    Dragon();
    break;
case 6:
    Baron();
    break;
    }

Other problems related to code-conventions (like the upper-case B in Baron, and D in Dragon are not as significant).

Case statements in Java are 'fall-through', and you need to break unless you want the following statements to execute as well.

Timer

In Java, the right solution for any timing problem, is to use the library timer functions. In recent Java versions, the right tool is the ScheduledExecutorService. This service allows you to schedule tasks in the future.

I would build it something like:

private final class TimedEvent {
    private final long preTarget;
    private final String message;
    TimedEvent(long millis, String message) {
        this.preTarget = millis;
        this.message = message;
    }
    private long delayTo(final long target) {
        return target - preTarget;
    }
}

.....

public static final TimedEvent[] EVENTS = {
    new TimedEvent(240000, "4 Minutes left"),
    new TimedEvent(180000, "3 Minutes left"),
    new TimedEvent(120000, "2 Minutes left"),
    new TimedEvent(60000,  "1 Minute left"),
    new TimedEvent(30000,  "30 Seconds left"),
    new TimedEvent(15000,  "15 Seconds left"),
    new TimedEvent(5000,   "5 Seconds left"),
    new TimedEvent(1000,   "1 Second left")
};

....

Then, with the TimedEvents set up, you can schedule them in a loop, counting down to the target time.....

final long now = System.currentTimeMillis();
final long target = now + sometime; /// whatever you are counting down to....
for (final TimedEvent te : EVENTS) {
    long delay = te.delayTo(target);
    if (delay >= 0) {
        Callable<Object> torun = new Callable<Void>() {
            public void call() {
                System.out.println(te.getMessage());
                return null;
            }
        }
        scheduler.schedule(torun, delay, TimeUnit.MILLISECONDS);
    }
}
\$\endgroup\$
0
12
\$\begingroup\$

You could pull out the while loop of each function and make it its own function. Something like this

private static void Countdown(long startTime, long endTime, string finalMsg){
    while (System.currentTimeMillis() / 1000 < endTime) {
        while (startTime != System.currentTimeMillis() / 1000) {
            startTime += 1;
            if (endTime - startTime > 1) {
                if (endTime - startTime == 240)
                    System.out.println("4 Minutes left");
                if (endTime - startTime == 180)
                    System.out.println("3 Minutes left");
                if (endTime - startTime == 120)
                    System.out.println("2 Minutes left");
                if (endTime - startTime == 60)
                    System.out.println("1 Minute left");
                if (endTime - startTime == 30)
                    System.out.println("30 Seconds left");
                if (endTime - startTime == 15)
                    System.out.println("15 Seconds left");
                if (endTime - startTime == 5)
                    System.out.println("5 Seconds left");
            } else if (endTime - startTime == 1) {
                System.out.println(finalMsg);
            }
        }
}

Then from each function, call the Countdown function. I think it will look a little cleaner and less repetitive.

\$\endgroup\$
3
  • 6
    \$\begingroup\$ I'd also precalculate endTime - startTime to improve performance. If done correctly, you can even shorten the parameter list then... \$\endgroup\$
    – Vogel612
    Commented Apr 24, 2014 at 14:42
  • 2
    \$\begingroup\$ @Vogel612 A decent compiler would make that optimisation for you. \$\endgroup\$
    – Pharap
    Commented Apr 24, 2014 at 19:38
  • 2
    \$\begingroup\$ A perfect example of why you should always write explicit braces. Also, the repetitive if statements should be chained using else if. Better yet, use a switch block. (I see @JerryCoffin has already addressed the issue.) \$\endgroup\$
    – 200_success
    Commented Apr 25, 2014 at 3:32
11
\$\begingroup\$

First: I think the point @DFord makes is quite good.

Second, I think you should work on indenting your code better. In particular, the indentation of your main if/then/else chain is misleading:

        if (endTime - startTime > 1)
            if (endTime - startTime == 360)
                System.out.println("6 Minutes left");
            if (endTime - startTime == 300)
                System.out.println("5 Minutes left");
            if (endTime - startTime == 240)
                System.out.println("4 Minutes left");
            if (endTime - startTime == 180)
                System.out.println("3 Minutes left");
            if (endTime - startTime == 120)
                System.out.println("2 Minutes left");
            if (endTime - startTime == 60)
                System.out.println("1 Minute left");
            if (endTime - startTime == 30)
                System.out.println("30 Seconds left");
            if (endTime - startTime == 15)
                System.out.println("15 Seconds left");
            if (endTime - startTime == 5)
                System.out.println("5 Seconds left");
        else if (endTime - startTime == 1) {
            System.out.println("Foo is up!");

When an else follows a chain of if statements like this, it attaches to the most recent if that doesn't already have an else. An if statement controls exactly one following statement (which needs to be a compound statement if you want it to control more than one syntactical statement). In other words, the code really works like:

if (endTime - startTime > 1)
    if (endTime - startTime == 360)
        System.out.println("6 Minutes left");

// The following are evaluated regardless of whether the `> 1` condition was met
if (endTime - startTime == 300)
    System.out.println("5 Minutes left");
if (endTime - startTime == 240)
    System.out.println("4 Minutes left");
if (endTime - startTime == 180)
    System.out.println("3 Minutes left");
if (endTime - startTime == 120)
    System.out.println("2 Minutes left");
if (endTime - startTime == 60)
    System.out.println("1 Minute left");
if (endTime - startTime == 30)
    System.out.println("30 Seconds left");
if (endTime - startTime == 15)
    System.out.println("15 Seconds left");
if (endTime - startTime == 5)
    System.out.println("5 Seconds left");
else if (endTime - startTime == 1) {
    System.out.println("Foo is up!");

Based on your indentation, you apparently wanted it to be more like this:

if (endTime - startTime > 1) {
    if (endTime - startTime == 360)
        System.out.println("6 Minutes left");
    if (endTime - startTime == 300)
        System.out.println("5 Minutes left");
    if (endTime - startTime == 240)
        System.out.println("4 Minutes left");
    if (endTime - startTime == 180)
        System.out.println("3 Minutes left");
    if (endTime - startTime == 120)
        System.out.println("2 Minutes left");
    if (endTime - startTime == 60)
        System.out.println("1 Minute left");
    if (endTime - startTime == 30)
        System.out.println("30 Seconds left");
    if (endTime - startTime == 15)
        System.out.println("15 Seconds left");
    if (endTime - startTime == 5)
        System.out.println("5 Seconds left");
}
else if (endTime - startTime == 1) {
    System.out.println("Foo is up!");

In this case, the braces prevent the else from attaching to the immediately previous if, forcing it to attach to the if you apparently intended instead.

Although it might initially seem (or even be) somewhat wasteful, I'd tend to avoid the issue entirely: instead of a cascade of if statements, I'd probably put the statements to be printed out into some kind of Map, then just do a lookup in the map and print out the associated value (if it has one) for a given time.

// In real code, you'll need to pick some class that implements the Map interface,
// but for the moment, I'm not really concerned with which one you pick.
Map<Integer, string> names;

names.put(360, "6 minutes left");
names.put(300, "5 minutes left");
//...
names.put(5, "5 seconds left");

Then the code just looks up the current time in names and prints out a string if there is one for a particular value. Aside: while I've used names here, it's probably not a good example to emulate--when naming the real variable, you should almost certainly pick a name that better reflects its intended function; something like timeToString, for example.

Also note that this makes it fairly easy to export the values and strings to an configuration file instead of them being embedded in the code. If, for example, you ended up wanting to translate the program to Russian or French (or whatever) translating these strings wouldn't (in and of itself) force re-compiling the associated code. Likewise, if you decided to add or remove some of the messages, moving them to an external data file makes this trivial to do.

That said, I should probably add that I doubt you could localize any significant program without re-compiling at all (or without rewriting at least a little of the code). Nonetheless, keeping strings like this external to the code does help keep the task a little more manageable.

\$\endgroup\$
0
10
\$\begingroup\$

Concept:

Your concept is not suited to be used on multiple countdowns, instead you might want to try something like the following:

Beware, this is a raw draft!

public class Buff{
    private int respawnSeconds;
    private String name;
    private int respawnsAtSecond;
    //constructor + getters and setters

    public void kill(int currentGameSeconds){
      this.respawnsAtSecond = currentGameSeconds + respawnSeconds;
    }
}

public class Game{
    private int elapsedSeconds;
    private Set<Buff> buffs;

    public void start(){
        elapsedSeconds = 0;
        buffs.add(new Buff("friendlyBlue", 300));
        //Add the other buffs
    }

    //You need some ticking mechanism, maybe you could use something like a while(true)
}

Sidenotes:

Naming:

Your method names are inconsistent.. the blue and red methods are camelCase, but your larger objectives are PascalCased (Dragon, Baron) aside from that, you are very consistent with very clear and understandable camelcased local variables.

Method Responsibilites:

You make your methods do too much. They are doing the counting, the printing and the calculations all at once... Extract sub-methods and add parameters for the different objective names.

\$\endgroup\$
6
\$\begingroup\$

I have never touched java, so this may not compile, but to make your code somewhat shorter, you could use, in addition to DFords answer (inside the loop):

long seconds = endTime - startTime;
if (seconds == 1)
    System.out.println(finalMsg);
else if (seconds == 5)
    System.out.println("5 Seconds left");
else if (seconds == 15)
    System.out.println("15 Seconds left");
else if (seconds == 30)
    System.out.println("30 Seconds left");
else if (seconds <= 240 && seconds % 60 == 0)
    System.out.println(seconds/60 + " minutes left");
\$\endgroup\$
2
  • \$\begingroup\$ Beat me to it. That should be 360 though, not 240. \$\endgroup\$
    – Pharap
    Commented Apr 24, 2014 at 19:48
  • \$\begingroup\$ @Pharap Hm. Seems like OP starts counting from multiple points (240s, 300s and 360s). \$\endgroup\$
    – Lennart_96
    Commented Apr 24, 2014 at 21:03
5
\$\begingroup\$

@Dford is completly right with putting the time event to an seperatly method.

I still find it more readable when you use the switch in combinations with readable final's

private static final int MINUTES_4 = 240;
private static final int MINUTES_3 = 180;
private static final int MINUTES_2 = 120;
private static final int MINUTES_1 = 60;
private static final int SECONDS_30 = 30;
private static final int SECONDS_15 = 15;
private static final int SECONDS_5 = 5;
private static final int SECONDS_1 = 1;

private static void Countdown(long startTime, long endTime, string finalMsg){
    while (System.currentTimeMillis() / 1000 < endTime) {
        while (startTime != System.currentTimeMillis() / 1000) {
            String message;
            startTime++;
            switch (endTime - startTime) {
               case MINUTES_4 : message = "4 Minutes left";
               break;
               case MINUTES_3 : message = "3 Minutes left";
               break;
               case MINUTES_2 : message = "2 Minutes left";
               break;
               case MINUTES_1 : message = "1 Minutes left";
               break;
               case SECONDS_30 : message = "30 seconds left";
               break;
               case SECONDS_15 : message = "15 seconds left";
               break;
               case SECONDS_5  : message = "5 seconds left";
               break;
               case SECONDS_1  : message = finalMsg;
               default : break;
            }
            if (null != message) {
                System.out.println(message);
            }
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

I modified your code a bit to work with several countdowns now:

  1. create an entity for your buffs
  2. create an map with buffs
  3. used the map to remove the switch statement in main function
  4. used the map to make the menu dynamic
  5. start a new thread for every timer to start more than one timer

package test;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Scanner;


public class LolTimer 
{       
    /**List with running countdowns*/
    private static List<String> countdowns = new ArrayList<String>();

    /**Main function*/
    public static void main(String[] args) 
    {
        //variable instantiation
        Scanner myScanner = new Scanner(System.in); 
        HashMap<Integer, Buff> BuffMap = creatBuffs();
        int numberBuffs = BuffMap.size();
        int buffID = 0;

        //Print the menue with buffs etc.
        System.out.println("Jungle Timers v1.0");
        System.out.println("\nSelect a buff to time:\n");
        for(int i = 1; i <= numberBuffs; i++)
            System.out.println(i + ". " + BuffMap.get(i).m_sName);
        System.out.println(numberBuffs+1 + ". EXIT");
        System.out.print("\n>  ");

        //loop to track as many buffs as the user want
        while(buffID != numberBuffs+1)
        {
            buffID = myScanner.nextInt();
            if(buffID > 0 && buffID <= numberBuffs)
            {
                Buff buff = BuffMap.get(buffID);
                trackBuff(buff.m_nRespawnTime, buff.m_sName);
            }
        }

        //if exit is choosen -> close scanner and exit the programm
        myScanner.close();
        System.exit(0);
    }

    /**Decide if a Buff is still running or not.*/
    public static void trackBuff(int nRespawnTime, String sName)
    {
        if(countdowns.contains(sName))
            System.out.println("This Countdown is still running.");
        else 
        {
            System.out.println("\nTracking " + sName + "...");
            startThread(nRespawnTime, sName);
        }
    }

    /**Starts new Thread with a new Countdowntimer.*/
    public static void startThread(int nRespawnTime, final String sName)
    {
        countdowns.add(sName);

        final int m_nStartTime = (int) (System.currentTimeMillis() / 1000);
        final int m_nEndTime = m_nStartTime + nRespawnTime;

        Thread updateThread = new Thread() 
        {
            @Override
            public void run() 
            {
                int curTime = m_nStartTime;
                int m_nTimeDiff = 0;

                while (System.currentTimeMillis() / 1000 < m_nEndTime) 
                {
                    while (curTime != System.currentTimeMillis() / 1000) 
                    {
                        m_nTimeDiff = m_nEndTime - curTime;
                        curTime += 1;
                        if (m_nTimeDiff == 420)
                            System.out.println(sName + ": 7 Minutes left");
                        if (m_nTimeDiff == 360)
                            System.out.println(sName + ": 6 Minutes left");
                        if (m_nTimeDiff == 300)
                            System.out.println(sName + ": 5 Minutes left");
                        if (m_nTimeDiff == 240)
                            System.out.println(sName + ": 4 Minutes left");
                        if (m_nTimeDiff == 180)
                            System.out.println(sName + ": 3 Minutes left");
                        if (m_nTimeDiff == 120)
                            System.out.println(sName + ": 2 Minutes left");
                        if (m_nTimeDiff == 60)
                            System.out.println(sName + ": 1 Minute left");
                        if (m_nTimeDiff == 30)
                            System.out.println(sName + ": 30 Seconds left");
                        if (m_nTimeDiff == 15)
                            System.out.println(sName + ": 15 Seconds left");
                        if (m_nTimeDiff == 5)
                            System.out.println(sName + ": 5 Seconds left");
                    }
                }

                System.out.println(sName + " is up!");
                countdowns.remove(sName);
                Thread.currentThread().interrupt();
            }
        };
        updateThread.start(); 
    }

    /**Creat all the Buffs you want. You fastly can add some Buffs(wolves, ghosts) here.*/
    public static HashMap<Integer, Buff> creatBuffs()
    {
        HashMap<Integer, Buff> BuffMap = new HashMap<>();
        BuffMap.put(1, new Buff(300, "your Blue"));
        BuffMap.put(2, new Buff(300, "enemy Blue"));
        BuffMap.put(3, new Buff(300, "your Red"));
        BuffMap.put(4, new Buff(300, "enemy Blue"));
        BuffMap.put(5, new Buff(300, "Dragon"));
        BuffMap.put(6, new Buff(300, "Baron"));
        return BuffMap;
    }
}

/**Entity for your Buffs*/
class Buff
{
    public int m_nRespawnTime;
    public String m_sName;

    public Buff(int time, String name)
    {
        m_nRespawnTime = time;
        m_sName = name;
    }
}
\$\endgroup\$
3
\$\begingroup\$

If we're talking proper class encapsulation or whatever, here's my way over-engineered version of what you could do.

Also, I'm a C# programmer, so some of the syntax may be a little off, but I've made some effort at least to use correct Java.

  • Keep your console prompts in the Main method
  • Create an enum that represents your different types of timers
  • Create what is essentially just a data repository class that holds your strings (like "Tracking enemy blue...") and other constant values (like your 300 that gets added to the timer).
    • Use meaningful names. Everything is static final. Something like @chillworld's answer.
    • Call it something like GlobalConstants, or whatever is meaningful to you.
  • Create a class that is responsible for managing your timer (call it Tracker or something. whatever.). This class would include:
    • A constructor that takes the above mentioned enum as a parameter
    • A private field to locally store said enum
    • Countdown method (from @DFord's answer) mixed with the optimization from @Lennart_96's answer
    • An initialize method to set your timer values and such based on the local enum value, and call a separate method to display your initial countdown message, also based on the enum value.

In the end you'd end up with something like:

public enum TimerType {
    FriendlyBlue, EnemyBlue...
}

public class MainProgram {
    private TimerType tType;
    public static void main(String[] args) {
        // Do your console prompts

        // Assign the timer type
        switch (timerChoice) {
            case 1: tType = TimerType.FriendlyBlue; break;
            case 2: ...
        }

        // Create the tracker object and kick off the timer
        new Tracker(tType);
    }
}

public class Tracker {
    private TimerType tType;
    private string initialTrackingMessage;
    ...

    public Tracker(TimerType timerType) {
        tType = timerType;
        InitializeTracker();
        CountDown();
    }

    private void InitializeTracker() {
        initialTrackingMessage = GetInitialTrackingMessageForTrackingType();
        // Initialize any other variables

        DisplayMessage(initialTrackingMessage);
    }

    private string GetInitialTrackingMessageForTrackingType() {
        string returnMessage;
        switch (tType) {
            case FriendlyBlue: returnMessage = GlobalConstants.TRACKING_BLUE; break;
            ...
        }
        return returnMessage;
    }

    private void DisplayMessage(string message) {
        System.out.println(message);
        // Any other logic you want to do
    }

    private void CountDown() {
        // Your countdown logic here
    }
}

public static class GlobalConstants() {
    // Messages
    public static final string TRACKING_BLUE = "Tracking your blue..."
    ...

    // Values
    public static final int INITIAL_TIMER_LENGTH = 300;
}

I'm sure you noticed that I like to use whole words in my coding. It's really nice for people who have to maintain your code when you're gone. You could also easily add a Start() method or something that takes care of the initialization and countdown, if you don't like the idea of doing it all from the constructor.

If you end up adding more details to each type of timer, this approach makes it much easier to convert your core class into an abstract class, or even an interface, as appropriate. Maybe even both. The Countdown method would be appropriate for an abstract class, but the InitializeTracker method might be more appropriate for an interface, unless you need a default implementation. Also, the methods and types (classes, enums) each perform only one function, which is an important pillar of good OO design.

EDIT

As an additional benefit (and to address your secondary issue), you can easily track multiple buffs with this approach, by keeping a collection of Tracker objects.

\$\endgroup\$
0
\$\begingroup\$

Let me just address your "repetitive" segment where you check how many minutes are left.. some simple math can trim down the size of those blocks:

int secLeft = endTime - startTime;
int minsLeft = secLeft/60;
if( secLeft % 60 == 0 ) // only happens at multiples of 60, eg 0,60,120,180..
{
    // Message of form "2 minutes left"
    String msg = Integer.toString(minsLeft) + " Minutes left";
}
else if( secLeft == 30 )
{
    // need special cases for the smaller values
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.