General programming - usage of classes

Question

Ok, recently i have written a "paint application" in Java. Small program similar to MS Windows Paint and apparently (according to some people that evaluated my source code) im not using classes properly in my application. Unfortunatelly i have no idea what could be wrong, and yet noone is in the mood to tell me what is incorrect.

I have "proper" classes that use the idea of inheritance:

PaintObject  
-ClosedObject  
--RectangleShape  
--OvalShape  
-OpenObject  
--PointShape  
--LineShape

But still i have no idea what could be wrong in code, what could lead to thinking that classes are not used properly...

The only thing what (in my opinion) could be wrong is the fact that im drawing by creating the new objects like: new RectagleObject(...) and new OvalShape(...) instead of creating one field objectToBeDrawn variable (maybe a PaintObject type) and then assign appropriate shape to it when the method executeOperation is called (obviously along with drawing of that particular shape)

Here is the code sample:

public void executeOperation(Graphics2D gr, int oper, boolean drawingMode){
    if(oper==1){
        new RectangleShape(startLocation, endLocation, borderColor, fillColor).draw(gr);
    }
    else if(oper==2){
        new OvalShape(startLocation, endLocation, borderColor, fillColor).draw(gr);
    }
    ....//more operations
}

Anyone has any idea what could be wrong in terms of general programming habits (unless my 'guess' is right? - if it is, please confirm my doubts). Or maybe there is something else? (Skip the part of oper==1 since i know that such values should be defined constant)

Any help greatly appreciated, im learning by myself and it is really hard to guess how professional programs are designed (and follow their pattern) since my experience is "limited" (actually none).

I think CodeReview on the Stack Exchange would be better? codereview.stackexchange.com — jn1kk
– jn1kk, Commented Jul 12, 2012 at 18:30

Zong · Accepted Answer · 2012-07-12 20:32:37Z

3

If you're drawing raster images, do you need to keep track of what the user is drawing? Perhaps you mean objects such as PenTool, RectangleTool, etc?

Right now, it seems like you create objects that persist and are redrawn to draw specific shapes. You can simply have the tool objects draw on an Image once when appropriate (just don't ever clear the graphics context). Just assign a particular tool to some activeTool field when the tool is selected.

class DrawingTool {
     public void draw(Graphics g) { }
}

class PenTool extends DrawingTool {
     public void draw(Graphics g) {
         // some drawing logic
     }
}

class RectangleTool extends DrawingTool {
     public void draw(Graphics g) {
         // some other drawing logic
     }
}

So instead of

oper = 1;

you would write

activeTool = new PenTool(); // or activeTool = PenTool.GetInstance();

or something similar. Now when you want to draw something, just invoke the draw method. As I stated before, you can do this simply by passing in an Image that you never clear:

class DrawingCanvas {
     Image image = createImage(GetWidth(), GetHeight());
     DrawingTool activeTool;

     // this is what gets invoked on every frame
     public void paint(Graphics g){
          g.drawImage(image, 0, 0, null);
     }
}

// when you need to draw something
// (not on every frame, but something new)
activeTool.draw(image.getGraphics());

edited Jul 12, 2012 at 20:32

answered Jul 12, 2012 at 18:32

Zong

6,2585 gold badges34 silver badges46 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Marian Pazioch Over a year ago

Yes i am drawing a raster images, and yes i do need to keep track what user is drawing. I have in my GUI a tool selector that influences the oper that defines the shape being drawn at the moment.

Zong Over a year ago

If you really require the "oper" field, use enums instead of ints. I don't why you would need to though. I'll update my answer.

Marian Pazioch Over a year ago

Ok, now i got it, but still i have a question about this new Objects(). Should i avoid creating those new OvalShape() over and over again (every time it is drawn)? Or maybe i should create some PaintObject field and modify it as i mentioned initially? Or maybe it is completely irrelevant?

Zong Over a year ago

Well, it's only created when you select the tool. Drawing is done by invoking the method. I added a small example in my answer. If you want to avoid allocations, you could declare a static field in each of the classes so you could use it like activeTool = SomeTool.GetInstance().

StriplingWarrior · Accepted Answer · 2012-07-12 18:36:38Z

3

if(oper==1){
    //...
}
else if(oper==2){
    //...
}

This is the sort of code that proper use of Object-Oriented programming should be able to avoid 99% of the time. Investigate why you need an oper integer at all. There's probably a pattern you can modify to eliminate the need for it. For example, if you're currently assigning an integer value to each button in the toolbar, you might instead make the button handler set the current OperationFactory to one that produces the object of the type associated with that button.

answered Jul 12, 2012 at 18:36

StriplingWarrior

158k29 gold badges261 silver badges326 bronze badges

Comments

Localghost · Accepted Answer · 2012-07-12 18:43:13Z

1

Each of your types (specifically OvalShape and RectangleShape) should both implement an "executeOperation" method. This method should override the "executeOperation" method in the base class (closed object).

This will get rid of the need to if(oper == 2)... else (oper == 2). It will just call the implementation that is relevant for the object's type.

answered Jul 12, 2012 at 18:43

Localghost

7203 silver badges7 bronze badges

Comments

squishy · Accepted Answer · 2012-07-12 18:33:36Z

0

from the small snippet you could probably make more use of polymorphism. like have a parent abstract class/interface that defines the executeOperation(Graphics2D gr, boolean drawingMode) method then extend/implement it for each operation you support (may be able to drop drawingmode too i can't see what it does though.) Can possibly put the graphics2D into a constructor too.

answered Jul 12, 2012 at 18:33

squishy

615 bronze badges

1 Comment

Marian Pazioch Over a year ago

Well each of Shapes implements DoDrawing interface that deals with drawing its particular instance, so i guess from this point of view, it is quite ok. (I did not post whole 500+ line of code since it seems like a overkill. I have shown the executeOperation method, since this is the particular place where something is not used properly, still no idea what)

Collectives™ on Stack Overflow

General programming - usage of classes

4 Answers 4

4 Comments

Comments

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

4 Comments

Comments

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related