views:

229

answers:

6

I have a source code that is needed to be converted by creating classes, objects and methods. So far, I've just done by converting the initial main into a separate class. But I don't know what to do with constructor and which variables are supposed to be private. This is the code :

import java.util.*;

public class Card{

private static void shuffle(int[][] cards){

    List<Integer> randoms = new ArrayList<Integer>();
    Random randomizer = new Random();

    for(int i = 0; i < 8;) {
      int r = randomizer.nextInt(8)+1;
      if(!randoms.contains(r)) {
        randoms.add(r);
        i++;
      }
    }

    List<Integer> clonedList = new ArrayList<Integer>();
    clonedList.addAll(randoms);
    Collections.shuffle(clonedList);
    randoms.addAll(clonedList);
    Collections.shuffle(randoms);

    int i=0;

    for(int r=0; r < 4; r++){
        for(int c=0; c < 4; c++){
            cards[r][c] = randoms.get(i);
            i++;
        }
    }
}

public static void play() throws InterruptedException {

    int ans = 1;
    int preview;
    int r1,c1,r2,c2;
    int[][] cards = new int[4][4];
    boolean[][] cardstatus = new boolean[4][4];
    boolean gameover = false;
    int moves;
    Scanner input = new Scanner(System.in);

    do{
        moves = 0;

        shuffle(cards);

        System.out.print("Enter the time(0 to 5) in seconds for the preview of the answer : ");
        preview = input.nextInt();

        while((preview<0) || (preview>5)){
            System.out.print("Invalid time!! Re-enter time(0 - 5) : ");
            preview = input.nextInt();
        }

        preview = 1000*preview;
        System.out.println(" ");

        for (int i =0; i<4;i++){
            for (int j=0;j<4;j++){

                System.out.print(cards[i][j]);
                System.out.print(" ");
            }
            System.out.println("");
            System.out.println("");
        }

        Thread.sleep(preview);

        for(int b=0;b<25;b++){
            System.out.println(" ");
        }

        for(int r=0;r<4;r++){
            for(int c=0;c<4;c++){
                System.out.print("*");
                System.out.print(" ");
                cardstatus[r][c] = false;
            }
            System.out.println("");
            System.out.println(" ");
        }

        System.out.println("");

        do{
            do{
                System.out.print("Please insert the first card row : ");
                r1 = input.nextInt();
                while((r1<1) || (r1>4)){
                    System.out.print("Invalid coordinate!! Re-enter first card row : ");
                    r1 = input.nextInt();
                }

                System.out.print("Please insert the first card column : ");
                c1 = input.nextInt();
                while((c1<1) || (c1>4)){
                        System.out.print("Invalid coordinate!! Re-enter first card column : ");
                        c1 = input.nextInt();
                }

                if(cardstatus[r1-1][c1-1] == true){
                    System.out.println("The card is already flipped!! Select another card.");
                    System.out.println("");
                }
            }while(cardstatus[r1-1][c1-1] != false);

            do{
                System.out.print("Please insert the second card row : ");
                r2 = input.nextInt();
                while((r2<1) || (r2>4)){
                    System.out.print("Invalid coordinate!! Re-enter second card row : ");
                    r2 = input.nextInt();
                }

                System.out.print("Please insert the second card column : ");
                c2 = input.nextInt();
                while((c2<1) || (c2>4)){
                    System.out.print("Invalid coordinate!! Re-enter second card column : ");
                    c2 = input.nextInt();
                }

                if(cardstatus[r2-1][c2-1] == true){
                    System.out.println("The card is already flipped!! Select another card.");
                }
                if((r1==r2)&&(c1==c2)){
                    System.out.println("You can't select the same card twice!!");
                    continue;
                }
            }while(cardstatus[r2-1][c2-1] != false);

            r1--;
            c1--;
            r2--;
            c2--;

            System.out.println("");
            System.out.println("");
            System.out.println("");

            for(int r=0;r<4;r++){
                for(int c=0;c<4;c++){

                    if((r==r1)&&(c==c1)){
                        System.out.print(cards[r][c]);
                        System.out.print(" ");
                    }
                    else if((r==r2)&&(c==c2)){
                        System.out.print(cards[r][c]);
                        System.out.print(" ");
                    }
                    else if(cardstatus[r][c] == true){
                        System.out.print(cards[r][c]);
                        System.out.print(" ");
                    }
                    else{
                        System.out.print("*");
                        System.out.print(" ");
                    }
                }
                System.out.println(" ");
                System.out.println(" ");
            }

            System.out.println("");

            if(cards[r1][c1] == cards[r2][c2]){
                System.out.println("Cards Matched!!");

                cardstatus[r1][c1] = true;
                cardstatus[r2][c2] = true;
            }
            else{
                System.out.println("No cards match!!");
            }

            Thread.sleep(2000);

            for(int b=0;b<25;b++){
                System.out.println("");
            }

            for(int r=0;r<4;r++){
                for(int c=0;c<4;c++){
                    if(cardstatus[r][c] == true){
                        System.out.print(cards[r][c]);
                        System.out.print(" ");
                    }
                    else{
                        System.out.print("*");
                        System.out.print(" ");
                    }
                }
                System.out.println("");
                System.out.println(" ");
            }

            System.out.println("");
            System.out.println("");
            System.out.println("");

            gameover = true;

            for(int r=0;r<4;r++){
                for( int c=0;c<4;c++){
                    if(cardstatus[r][c]==false){
                        gameover = false;
                        break;
                    }
                }
                if(gameover==false){
                    break;
                }
            }

            moves++;

        }while(gameover != true);

        System.out.println("Congratulations, you won!!");
        System.out.println("It required " + moves + " moves to finish it.");
        System.out.println("");
        System.out.print("Would you like to play again? (1=Yes / 0=No) : ");
        ans = input.nextInt();

    }while(ans == 1);


}


}

The main class is:

import java.util.*;

public class PlayCard{

public static void main(String[] args) throws InterruptedException{

    Card game = new Card();
    game.play();

    }
}

Should I simplify the Card class by creating other classes?

Through this code, my javadoc has no constructtor. So i need help on this!

A: 

Unfortunately, the best (only) way to convert a "procedural" program to be "OO" is to build it from scratch.

Design your object model and the functionality you think each object should have and write the classes and method stubs.

Then you can possibly go back to your original code and pull some of this functional out of the procedure(s) and into the objects.

Robin Day
but the point is that i don't know which variables should be the private or public. And what should be the constructor for each class. That's what made me blur for hours..
keitamike
My point (probably unclear) is that OO programming and Procedural programming are a different design altogether and not just making variables public/private and encapsulating methods into classes. Therefore to "begin" you should design your object model. List all possible objects, such as, Card, Board, Game, etc. and how they fit together and interact with each other.
Robin Day
I disagree. Refactoring is almost always cheaper than re-writing. Take small steps identifying concepts that have usefulness in OO - that is, they can be encapsulated. Factor those out into new objects... and make sure the code still runs. If you write from scratch, you'll likely not have anything working for a long time - this is generally unacceptable in industry... and leads many rewrite projects to fail.
Jason R. Coombs
This isn't refactoring, this is effectively changing the language. I agree that you should re-use as much of the code as possible but if you want/need to follow an OO approach then you have no choice but to design an object model first.
Robin Day
@Robin Agreed. The approach needs to be considered based on the problem, not on the existing code... but once a good approach is designed, it should be possible to refactor into that domain, maintaining functionality all the way. Sometimes, that's not possible - but usually it is.
Jason R. Coombs
A: 

All kinds of smells can be found in this code. If you really want to make nice code from it that read Fowler's Refactoring chapter by chapter and apply all gained knowledge immediately.

Another approach is simply don't touch it, "don't repair working engine".

Roman
+6  A: 

It looks like the classic memory game, using cards. I see four main classes:

  1. Game
  2. Deck
  3. Card
  4. Grid

Here are some suggested properties and methods for each. It's incomplete, but it should get you going.

Game
  previewDuration
  selectedRow
  selectedColumn
  selectedMatchRow
  selectedMatchColumn
  deck
  grid
  main()
  promptForColumn()
  promptForRow()
  preview()
  loadGrid()

Deck
  cardCount
  shuffle()
  getNextCard()

Card
  caption
  equals()

Grid
  width
  height
  getCardAtPos()
Marcus Adams
For the visual-thinkers out there (myself included) what would be awesome if someone could post a picture of this as a UML conceptual model.
Kelly French
+1  A: 

This answer illustrates one possible refactoring you can do to make your approach more object-oriented. It doesn't address the problem domain (Cards, Decks, etc), which are addressed by other answers. Instead, it describes how you might use OO principles to tackle the programmatic implementation in a more OO manner.

The first step I would do is factor out what I'll call the PromptedLoop construct. You have a do/while loop in .play that prompts the user whether or not to run, and while he says yes, it does some action.

Take this functionality and encapsulate it. Make the Card a Runnable object and allow a PromptedLoop to run it repeatedly.

public interface Runnable
{
    public void Run();
}

public class PromptedLoop()
{
    private Runnable runner;
    private String prompt;

    public PromptedLoop(Runnable runner)
    {
        this.runner = runner;
        this.prompt = "Again? (1=Yes / 0=No):";
    }

    public PromptedLoop(Runnable runner, String prompt)
    {
        this.runner = runner;
        this.prompt = prompt;
    }

    public void Go()
    {
        int ans = 0;
        do
        {
            runner.Run();
            System.out.println("");
            System.out.print(prompt);
            ans = input.nextInt();
        } while(ans == 1);
    }
}

class Card
implements Runnable
{
    public void Run()
    {
        play();
    }
    ...
}

public class PlayCard {
    public static void main(String[] args) throws InterruptedException
    {
        Card game = new Card();
        PromptedLoop loop = new PromptedLoop(game, "Would you like to play again? (1=Yes, 0=No)");
        loop.Go();
    }

Then, .play can be just a single instance of a play action and doesn't have to integrate the looping functionality. You then have the option of re-using the PromptedLoop for other programs, and you can more easily read .play as you're not distracted by the fact that it might be looped over.

Jason R. Coombs
+2  A: 

Before worrying about objects, you should first at least separate out sections into named methods so it's more clear what each part is doing. That's important whether it's OO or not. For instance:

    for (int i =0; i<4;i++){
        for (int j=0;j<4;j++){

            System.out.print(cards[i][j]);
            System.out.print(" ");
        }
        System.out.println("");
        System.out.println("");
    }

looks like it's printing out the board. Make that a separate function and call it printBoard or something similar.

As for classes, I'd probably make a Board class that has your cards and cardstatus and methods to check a current card as well probably shuffle. After you get that working I'd try to remove all input and output from the Board class.

Refactoring is rarely a single-step process. Rewriting is an option since it's a relatively small program, but be aware that you'll probably reintroduce some bugs you previously fixed and probably some new ones in the process (same thing with refactoring, but making smaller changes makes it slightly less likely)

Davy8
+3  A: 

But I don't know what to do with constructor and which variables are supposed to be private.

Whatever can be, should be private. To determine this, it's often easiest to mark everything private, and then when you hit a "pain point" promote it to public.

As for what classes you need, take a look at the existing code for "smells" such as:

int[][] cards = new int[4][4];

You have a "named primitive"1 there - and it's used quite often. This makes it a pretty important noun to your program. Encapsulate it into a class:

public class Cards {
   private int[][] cards = new int[4][4];
}

Now, look for where you're manipulating that named primitive:

shuffle(cards);

for (int i = 0; i < 4; i++){
   for (int j = 0; j < 4; j++){
       System.out.print(cards[i][j]);
       System.out.print(" ");
   }
   System.out.println("");
   System.out.println("");
}

Those are prime targets for methods:

public class Cards {
   private int[][] cards = new int[4][4];

   public void shuffle() {
      // existing shuffle method goes here - but works with private cards
   }

   public void print() {
      for (int i = 0; i < 4; i++){
        for (int j = 0; j < 4; j++){
           System.out.print(cards[i][j]);
           System.out.print(" ");
        }
      System.out.println("");
      System.out.println("");
   }
}

And, then - look at easy ways to generalize. Cards currently is hardcoded to a 4 x 4 board - let's parameterize that:

public class Cards {
   private int width;
   private int length;
   private int[][] cards;

   public void shuffle() {
      // existing shuffle method goes here - but works with private cards
   }

   public void print() {
      for (int i = 0; i < length; i++){
        for (int j = 0; j < width; j++){
           System.out.print(cards[i][j]);
           System.out.print(" ");
        }
      System.out.println("");
      System.out.println("");
   }
}

Now we need someone to provide the length and width - that's what constructors are for:

public class Cards { private int width; private int length; private int[][] cards;

   public Cards(int length, int width) {
      this.length = length;
      this.width = width;
      this.cards = new int[length][width];
   }

   public void shuffle() {
      // existing shuffle method goes here - but works with private cards
   }

   public void print() {
      for (int i = 0; i < length; i++){
        for (int j = 0; j < width; j++){
           System.out.print(cards[i][j]);
           System.out.print(" ");
        }
      System.out.println("");
      System.out.println("");
   }
}

And, then we realize that Cards isn't such a good name for this...It's more of a Board - rename it and move on to the next "named primitive" or smell2.

1 I use "named primitive" to indicate the same primitive type that is either global or passed around between methods with the same name. Since there is no class, the semantic meaning is purely in the name - often that name is a great starting point for a class. It's related to the well-known "primitive obsession" code smell, but slightly different in that it doesn't require a large number of primitive types masquerading as a class. A single named primitive can be promoted to a class.

2 A lot of the code smells are particular to OOP code. For looking at procedural code that you're trying to convert to OO, I think "primitive obsession", "data clumps", "message chains", and "long parameter lists" are the most relevant.

Mark Brackett
+1 Good tips, but instead of promoting from private to public, first promote to package-private, then protected/public
Helper Method