Hey guys I had some homework in a Java class on Friday. I completed it so I am not looking for answers (I also don't think it will be graded). I was just curious, as a learning exercise, how would you guys improve this and why? Now remember I'm a java newb so go easy; I still haven't learned what an object is. I'm sure their has to be a better, prettier way of doing this though so I figured I would ask the pros. Here's the code:

/*
 * This program generates and prints a 
 * random card from a 52 card deck
 */
package randomcard;

/**
  * @author 24x24
 */
public class Main {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {

        int cardRank = randomRank(); // calls the randomRank method
        int suitNumber = randomSuit(); // calls the randomSuit method
        String cardSuit = suits(suitNumber); // calls the suits method
        cards(cardRank, cardSuit); // calls the cards method

    } // end of main method

/**
 * This method takes the variable suitNumber and
 * uses it to find variable cardSuit
 * @param suitNumber
 * @return cardSuit
 */
    public static String suits(int suitNumber) {

        // Declare variable cardSuit
        String cardSuit = "null";

        if (suitNumber == 1) {
            cardSuit = "Spades";
        } else if (suitNumber == 2) {
            cardSuit = "Hearts";
        } else if (suitNumber == 3) {
            cardSuit = "Clubs";
        } else if (suitNumber == 4) {
            cardSuit = "Diamonds";
        }


        return cardSuit;
    } // end of suits method

    /**
     * This method tells the user which card they have recieved
     * by using variables cardRank and cardSuit. If they cardRank
     * is 1,11,12, or 13 it gives a face value
     * @param cardRank
     * @param cardSuit
     */

    public static void cards(int cardRank, String cardSuit) {
        // if statement set finds face cards and gives them a name
        if (cardRank == 1) {
            System.out.println("Your card is an Ace of " + cardSuit);
        } else if (cardRank == 13) {
            System.out.println("Your card is a King of " + cardSuit);
        } else if (cardRank == 12) {
            System.out.println("Your card is a Queen of " + cardSuit);
        } else if (cardRank == 11) {
            System.out.println("Your card is a Jack of " + cardSuit);
        } else if (cardRank == 8) { // 8 is the only number that requires 'an'
            System.out.println("Your card is an " + cardRank + " of " + cardSuit);
        } else {
            // Print out the randomly selected card
            System.out.println("Your card is a " + cardRank + " of " + cardSuit);

        }
    } // end of cards method

    /**
     * This method randomly creates the variable cardRank
     * @return cardRank
     */

    public static int randomRank() {

        int cardRank = (int) ((int) 1 + (Math.random() * (13 - 1 + 1))); // creates a random card rank

        return cardRank;

    } // end of randomRank method

    /**
     * This method randomly creates the variable suitNumber
     * @return suitNumber
     */

    public static int randomSuit() {

        int suitNumber = (int) ((int) 1 + (Math.random() * (4 - 1 + 1))); // creates a random card rank

        return suitNumber;

    } // end of randomRank method
} // end of Main class

It's long which is mostly what I think could be improved, but I'm not too sure how one would do that. I promise this is complete; I am not trying to get answers. If you don't believe me look at my other posts where I've been trying to learn and not just ask for the answer. I just want to know how to do this better since I finished in record time and didn't need any help!
Thanks for any input!

Yeah, it's a bit long, and there are other ways to solve what you are achieving, but overall, not a bad job!

As far as not having learned what an object is, you're already using them. Your class is an object. String is an object. Math.random() is a (static) method on an object.

It's easy to get trapped into the if/then/else way of approaching a problem. Consider using a switch statement instead. For example, your cards method could be cleaned up a bit:

public static void cards(int cardRank, String cardSuit) {
		// if statement set finds face cards and gives them a name
		String yourCard = "Your card is a";
		String result = null;
		switch (cardRank) {
		case 1:
			result = yourCard + "n Ace of " + cardSuit;
			break;
		case 8:
			result = yourCard + "n 8 of " + cardSuit;
			break;
		case 11:
			result = yourCard + "Jack of " + cardSuit;
			break;
		// And so on...

		default:
			result = yourCard + " " + cardRank + " of " + cardSuit;
			break;
		}

		System.out.println(result);
	} // end of cards method

You can do the same when assigning the suit. The switch statement makes things a little more readable, and performs better.

Avoid excessive String concatenation where possible - in a large or complex program it can gobble up a lot of memory and cause the garbage collector to run more often (a bad thing.)

You could go so far as to create classes representing Suit and Rank, which could conceivably give you more flexibility, but in the context of this exercise, simplicity is better and seems just fine to me.

Hopefully this was helpful.

Hey thanks for the input. I was right, it wasn't graded. The professor just wanted to prove a point on how we could all be more efficient with our code and then he went into how objects could allow for this and it makes it easier to write easily modified code. I like what you did with the switch statement. I always forget about those. However, the profs ultimate goal was to get us to use two arrays (one with a list of suits, one with ranks), randomly pull a suit and rank variable and output it in a string. All in all 5-6 lines of code where I had close to 80. Thanks again!

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.