2
\$\begingroup\$

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();
    }
}

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. f I do not, please tell me why, and how else I can improve this code. Also, I have other classes besides Ogg and OggConverter; I also have Mp3 and Mp3Converter.

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

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;
 }
}
\$\endgroup\$
4
  • \$\begingroup\$ if I did that, would it be tightly coupled? \$\endgroup\$
    – user962206
    Commented Jan 3, 2013 at 13:05
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jan 3, 2013 at 13:16
  • \$\begingroup\$ 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? \$\endgroup\$
    – user962206
    Commented Jan 3, 2013 at 14:00
  • \$\begingroup\$ You can use the name method or create own definition, see my edit. \$\endgroup\$ Commented Jan 3, 2013 at 14:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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