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.

This is my command interface

public interface IConverter {
    void convert();
}

This is my Receiver class

public class Ogg extends Audio{

    private File src;
    private File trgt;

    public static final String CODEC = "libvorbis";
    public static final String FORMAT = "ogg";

    public Ogg(File src, File trgt){
        this.src = src;
        this.trgt = trgt;
    }

    public void convertToOgg(){
        audioAttr.setCodec(CODEC);
        encoAttrs.setFormat(FORMAT);
        encoAttrs.setAudioAttributes(audioAttr);
        try {
            encoder.encode(src, trgt, encoAttrs);
        } catch (IllegalArgumentException e) {
             e.printStackTrace();
        }
    }
}

And this is my Concrete Command

package org.hitplay.audio.converters;

public class OggConverter implements IConverter {

    private Ogg ogg;

    public OggConverter(Ogg ogg){
        this.ogg = ogg;
    }
    @Override
    public void convert() {
        ogg.convertToOgg();
    }
}

And lastly this is my Invoker class.

public class AudioConverter {
    IConverter audio;

    public AudioConverter(IConverter audi){
        this.audio = audi;
    }
    public void setAudio(IConverter audio){
        this.audio = audio;
    }
    public void convert(){
        audio.convert();
    }
}

I have currently studied the Command Design Pattern on this link

And I was wondering if I have implemented the Design Pattern Correctly, if I do not please tell me why, and how else can I improve this code? also I have other classes besides Ogg and OggConverter I also have Mp3 and Mp3Converter

share|improve this question
    
If you implement design patterns, make transparent to the reader what you are doing. Here: be sure to append Command to the classes that are commands. And work on your accept rate. –  burna Jan 3 '13 at 11:53
add comment

1 Answer

I do not see the reason of for class OggConverter as it is now.

Instead of that you could just add the interface IConverter to class Ogg. And remove all others classes as then only forward the call.

IConverter converter = OggConverter(Ogg(file1,file2));

If we add the interface to Ogg

IConverter converter = Ogg(file1,file2);

I also do not see the reason for class AudioConverter, beside that it can store the ref to IConverter but we can store this reference in other place.

In addition to improve are those CODEC and FORMAT string constant. Instead of string constatns you shuld provide enumerations.

public enum AudioCodec {
  LIBVORBIS;
}


public enum AudioFormat {
 OGG("ogg"),
 MP3("mp3");

private final String ext; 

private AudioFormat(String ext) {
  this.ext = ext;
}


public String getExt() {
   return this.ext;
 }
}
share|improve this answer
    
if I did that, would it be tightly coupled? –  user962206 Jan 3 '13 at 13:05
    
No. Losse couplind is not based about cration as many classes as posibile. It about creation the classes that do not depend on each other. For that you should have class that know how to convert a file. and a class that provide the definition. –  Vash Jan 3 '13 at 13:16
    
About the enums, the codecs and Formats is required to be an mp3. not an enum, if I decide to make it as an enum how would I then provide it as a String rather than an enum itself? –  user962206 Jan 3 '13 at 14:00
    
You can use the name method or create own definition, see my edit. –  Vash Jan 3 '13 at 14:20
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.