Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I came across an exercise (in the book "The Art and Science of Java" by Eric Roberts) that requires using only GArc and GLine classes to create a lettering library which draws your initials on the canvas. This should be made independent of the GLabel class.

Write a GraphicsProgram to draw your initials on the graphics window using only the GArc and GLine classes rather than GLabel. For example, if I wrote this program, I would want the output to be

[an image showing simple letters E S R on a canvas]

Think about the best decomposition to use in writing the program. Imagine that you’ve been asked to design a more general letter-drawing library. How would you want the methods in that library to behave in order to make using them as simple as possible for your clients?

I'd like to know the correct approach to use in solving this problem. I'm not sure what I have so far is good enough (I'm thinking it's too long). The questions requires that I use a good Top-Down approach.

Here's my code so far:

//Passes letters to GLetter objects and draws them on the canvas
package artScienceJavaExercises.chapter8;

    package artScienceJavaExercises.chapter8;

    import acm.program.*;
    //import acm.graphics.*;

    public class DrawInitials extends GraphicsProgram{

        public void init(){
            resize(400,400);
        }

        public void run(){
            //String let = readLine("Letter?: ");
            letter = new GLetter("l");
            add(letter, (getWidth()-letter.getWidth()*2)/2, (getHeight()-letter.getHeight())/2);

            add(new GLetter("o"), (letter.getX()+letter.getWidth()), letter.getY());
        }
        private GLetter letter;
    }

//GLetter Class
    package artScienceJavaExercises.chapter8;

    import acm.graphics.*;
    import java.awt.*;

    public class GLetter extends GCompound{

        private static final int ONE_THIRD = 30;
        private static final int ROW_2_HEIGHT = 40;


        private GArc[] arc = new GArc[4];
        private GLine[] line = new GLine[24];

        public GLetter(String s){
            line[0] = new GLine(0,0, ONE_THIRD, 0);
            line[1] = new GLine(ONE_THIRD,0, ONE_THIRD*2, 0);
            line[2] = new GLine(ONE_THIRD*2,0, ONE_THIRD*3, 0);
            line[3] = new GLine(0,0, 0,ONE_THIRD);
            line[4] = new GLine(ONE_THIRD,0, ONE_THIRD, ONE_THIRD);
            line[5] = new GLine(ONE_THIRD*2,0, ONE_THIRD*2, ONE_THIRD);
            line[6] = new GLine(ONE_THIRD*3,0, ONE_THIRD*3, ONE_THIRD);
            line[7] = new GLine(0,ONE_THIRD, ONE_THIRD*2, ONE_THIRD);
            line[8] = new GLine(ONE_THIRD,ONE_THIRD, ONE_THIRD*2, ONE_THIRD);
            line[9] = new GLine(ONE_THIRD*2,ONE_THIRD, ONE_THIRD*3, ONE_THIRD);
            line[10] = new GLine(0,ONE_THIRD, 0, ONE_THIRD+ROW_2_HEIGHT);
            line[11] = new GLine(ONE_THIRD, ONE_THIRD, ONE_THIRD, ONE_THIRD+ROW_2_HEIGHT);
            line[12] = new GLine(ONE_THIRD*2,ONE_THIRD, ONE_THIRD*2, ONE_THIRD+ROW_2_HEIGHT);
            line[13] = new GLine(ONE_THIRD*3,ONE_THIRD, ONE_THIRD*3, ONE_THIRD+ROW_2_HEIGHT);
            line[14] = new GLine(0, ONE_THIRD+ROW_2_HEIGHT, ONE_THIRD, ONE_THIRD+ROW_2_HEIGHT);
            line[15] = new GLine(ONE_THIRD, ONE_THIRD+ROW_2_HEIGHT, ONE_THIRD*2, ONE_THIRD+ROW_2_HEIGHT);
            line[16] = new GLine(ONE_THIRD*2, ONE_THIRD+ROW_2_HEIGHT, ONE_THIRD*3, ONE_THIRD+ROW_2_HEIGHT);
            line[17] = new GLine(0, ONE_THIRD+ROW_2_HEIGHT,  0, ONE_THIRD*2+ROW_2_HEIGHT);
            line[18] = new GLine(ONE_THIRD, ONE_THIRD+ROW_2_HEIGHT,  ONE_THIRD, ONE_THIRD*2+ROW_2_HEIGHT);
            line[19] = new GLine(ONE_THIRD*2, ONE_THIRD+ROW_2_HEIGHT,  ONE_THIRD*2, ONE_THIRD*2+ROW_2_HEIGHT);
            line[20] = new GLine(ONE_THIRD*3, ONE_THIRD+ROW_2_HEIGHT, ONE_THIRD*3, ONE_THIRD*2+ROW_2_HEIGHT);
            line[21] = new GLine(0,ONE_THIRD*2+ROW_2_HEIGHT, ONE_THIRD, ONE_THIRD*2+ROW_2_HEIGHT);
            line[22] = new GLine(ONE_THIRD, ONE_THIRD*2+ROW_2_HEIGHT, ONE_THIRD*2, ONE_THIRD*2+ROW_2_HEIGHT);
            line[23] = new GLine(ONE_THIRD*2,ONE_THIRD*2+ROW_2_HEIGHT, ONE_THIRD*3, ONE_THIRD*2+ROW_2_HEIGHT);

            for(int i = 0; i<line.length; i++){
                add(line[i]);
                line[i].setColor(Color.BLACK);
                line[i].setVisible(false);
            }

            arc[0] = new GArc(getWidth(), getHeight(), 106.699, 49.341);
            arc[1] = new GArc(getWidth(), getHeight(), 23.96, 49.341);
            arc[2] = new GArc(getWidth(), getHeight(), -23.96, -49.341);
            arc[3] = new GArc(0,0,getWidth(), getHeight(), -106.699, -49.341);

            for(int i = 0; i<arc.length; i++){
                add(arc[i],0,0);
                arc[i].setColor(Color.BLACK);
                arc[i].setVisible(false);
            }

            paintLetter(s);
        }

        private void paintLetter(String s){
            if (s.equalsIgnoreCase("l")){
                turnOn(line[3]);
                turnOn(line[10]);
                turnOn(line[17]);
                turnOn(line[21]);
                turnOn(line[22]);
                turnOn(line[23]);
            }
            else if(s.equalsIgnoreCase("o")){
                for(int i = 0; i<4; ++i){
                    turnOn(arc[i]);
                }
                turnOn(line[1]);
                turnOn(line[10]);
                turnOn(line[13]);
                turnOn(line[22]);
            }
        }

        private void turnOn(GObject g){
            g.setVisible(true);
        }


    }

I created a class (GLetter.java) with arrays for GArc and GLine objects. They are positioned in certain ways thereby turning certain Glines and/or GArcs on or off (changing visiblity) would create a pattern for a letter. This Gletter uses the if/else statements to determine which pattern to create - this makes me feel my code is too long.

There is another class (DrawInitials.java) that simulates a GraphicsProgram and allows the user to pass certain letters as arguments to the GLetter object. I've used 'L' and 'O' as examples.

However, I posted this because I'm not sure I'm using the right approach. That's why I need your help.

I feel MY CODE IS TOO LONG!

The code above is not the complete project...it only draws letters 'L' and 'O' for now.

share|improve this question
add comment

3 Answers 3

up vote 2 down vote accepted

Your answer is too long because you are trying to do more than you were asked to.

public class DrawInitials extends GraphicsProgram{
    public void run(){
        drawLetterL();
        drawLetterO();
    }

    public void drawLetterL(){
         add(new GLine( ... )); // Vertical Stroke
         add(new GLine( ... )); // Horizontal Stroke
    }

    public void drawLetterO(){
         add(new GArc( ... )); // A circle
    }
}

How would you generalize this. There may be many answer to this question but an answer similar to the one below is expected I guess:

public abstract class LetterDrawer {
    // this is to show that you cannot hard-code 
    // coordinates in a generalized graphics routine

    abstract void draw(GObject o, double xOffset, double yOffset);
}

// this is the class *your clients* will actually use
public class LetterDrawingService {
    Map<Character, LetterDrawer> letterDrawers;

    LetterDrawingService () {
          // initialize your drawer map
    }

    public draw(GObject o, char c, double xOffset, double yOffset) {
         // Validate as in the @Palacsint's answer
         letterDrawers.get(c).draw(o, c, xOffset, yOffset);
    }
}

I doubt book teaches factory classes before arrays, so I will not give an example for that case. Basic Idea is you have a class for each letter which extends some abtract GLetter and pass Offset Position to the constructor. (Side note: you should have a position class, if the graphics library doesn't have them).

I do not know acm.grahics package so do not get hung up on the details.

share|improve this answer
add comment
  1. You might want to remove the unused lines. The code does not use line[2], line[4], line[5], line[18] etc.

  2. You should check the input value of the GLetter class. Currently it does not show, for example, letter a, it would be nice to tell the users that's not their fault that the class ignores this letter. I'd throw an IllegalArgumentException in this case. (Effective Java, Second Edition, Item 38: Check parameters for validity)

  3. You might want to change the input type of the GLetter class to char. Does it make sense to call it with more than one character?

  4. I'd create a map with the letters and the required segments for each letter:

    final Multimap<Character, GObject> segmentsMap = HashMultimap.create();
    segmentsMap.putAll('l', Lists.newArrayList(line[3], line[10], line[17], 
        line[21], line[22], line[23]));
    segmentsMap.putAll('o', Lists.newArrayList(line[1], line[10], line[13], 
        line[22], arc[0], arc[1], arc[2], arc[3]));
    

    Then all you need is a for loop to make visible the required segments:

    if (!segmentsMap.containsKey(c)) {
        throw new IllegalArgumentException("Invalid letter: " + c);
    }
    
    final Collection<GObject> segments = segmentsMap.get(c);
    for (final GObject segment: segments) {
        turnOn(segment);
    }
    

    (I've used Guava's Multimap here but you can do this with multidimensional arrays too.)

share|improve this answer
    
Thanks for the response. Like I said, the code is still incomplete. By the time I fully implement the entire GLetter class, those unused GLine objects would become useful because the class would create an entire "letter library". Also, it is completely independent of GLabel class. This question comes before we are being taught about arrays, which is one of the things that make me feel my code is being wrongly implemented. So, I don't think the use of segments is appropriate in solving this problem. –  Ace Takwas Nov 10 '12 at 5:57
add comment

A code like this is never TOO LONG, if each letters have différent parameters and different algoritmes.
Easy to maintain without disturbing other letters for maintenaing a letter, not difficult to repeat on each letter if necessary(just to pay attention, and a second reader/controler).

So if you have time to parametrize all, few classes are necessary, choosing good design pattern may be difficult.

Letter parameters will be in letterPrmFile.properties/globalPrm.properties.
Changing .properties is easy, but changing a method, a condition, a single bit in the code need to prepare many jUnit testcases, and many tests in many different types of windows.

It is up to you under your constraints.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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