I have this code that converts audio to different file formats:
import java.io.File;
import it.sauronsoftware.jave.AudioAttributes;
import it.sauronsoftware.jave.Encoder;
import it.sauronsoftware.jave.EncoderException;
import it.sauronsoftware.jave.EncodingAttributes;
import it.sauronsoftware.jave.InputFormatException;
/* required jars(jave-1.0.2.jar)
* Handles Encoding and Decoding
* Audio to different file formats
*/
public class AudioEncoderDecoder {
private static final Integer bitrate = 256000;//Minimal bitrate only
private static final Integer channels = 2; //2 for stereo, 1 for mono
private static final Integer samplingRate = 44100;//For good quality.
/* Data structures for the audio
* and Encoding attributes
*/
private AudioAttributes audioAttr = new AudioAttributes();
private EncodingAttributes encoAttrs = new EncodingAttributes();
private Encoder encoder = new Encoder();
/*
* File formats that will be converted
* Please Don't change!
*/
private String oggFormat = "ogg";
private String mp3Format = "mp3";
private String wavFormat = "wav";
/*
* Codecs to be used
*/
private String oggCodec = "vorbis";
/* Set the default attributes
* for encoding
*/
public AudioEncoderDecoder(){
audioAttr.setBitRate(bitrate);
audioAttr.setChannels(channels);
audioAttr.setSamplingRate(samplingRate);
}
public void encodeAudio(File source, File target, String mimeType){
//Change the hardcoded mime type later on
if(mimeType.equals("audio/mp3")){
this.mp3ToOgg(source, target);
}
}
private void mp3ToOgg(File source, File target){
//ADD CODE FOR CHANGING THE EXTENSION OF THE FILE
encoAttrs.setFormat(oggFormat);
audioAttr.setCodec(oggCodec);
encoAttrs.setAudioAttributes(audioAttr);
try{
encoder.encode(source, target, encoAttrs);
}catch(Exception e){
System.out.println("Encoding Failed");
}
}
private void oggToMp3(){
//ADD CODE FOR CHANGING THE EXTENSION OF THE FILE
encoAttrs.setFormat(mp3Format);
audioAttr.setCodec(mp3Codec);
encoAttrs.setAudioAttributes(audioAttr);
try{
encoder.encode(source, target, encoAttrs);
}catch(Exception e){
System.out.println("Encoding Failed");
}
}
public static void main(String[] args){
AudioEncoderDecoder aed = new AudioEncoderDecoder();
File source = new File("singelementsmp3.mp3");
File target = new File("test.ogg");
//Test Mp3 To Ogg Convertion
String mimeType = "audio/mp3";
aed.encodeAudio(source, target, mimeType);
//Test Ogg To Mp3 Convertion
}
}
Now, the xToX conversion basically the same throughout the whole code implementation. The only thing will change is the codec.
What is another way to make this code cleaner? From my point of view it repeats itself. What should I change? Or are there any design patterns I should implement?
2 Answers 2
Overall
Design first.
Name and semantic of your class
I have this code that converts audio to different file formats.
Not exactly, your code is a client to an Encoder
, that converts audio to different file formats.
In design, it is very important to be certain about the reason and semantic of an object. So first, the name AudioEncoderDecoder
is bad. It says nothing about the sematic of the object. I would call it AudioFormatConverter
as it converts one audio file format to another.
Public methods
public void encodeAudio(File source, File target, String mimeType);
a) Naming encodeAudio()
is not clear, convert()
would be clear and consistent with the classname.
b) Parameters: source, target file seems good. But why would I pass in the mimeType? And how can I specify which target format I choose?
Expected method signature:
public void convert(File source, File target) throws AudioConversionException();
One conversion method in the public interface, source and target file, the mime type is guessed by the file extension. You could add another public method, where you take in the mimetype, as needed. But keep the interface of your class as simple, logic, consistent and understandable as possible.
I would declare an checked exception for error handling, formats not avail or whatever.
Design Patterns
If there will be more formats, codecs, mimetypes you could take a look at Chain of responsibility or Dispatcher/Command pattern. These patterns also add flexibility to easily support more mimetypes at runtime. Without those patterns, you may get a mid-to-big if/then/else block.
Possible Design with Command, Strategy pattern applied: Possible design of AudioFormatConverter
AudioFormatConverter
s interface is format-agnostic, this means it is possible to implement a converter, that doesn't use Files but only InputStreams or byte[] arrays.- Concrete
AudioFileFormatConverter
converts from and toFile
s, and holds a list of supportedConversionCommand
s ConversionCommand
encapsulates logic to convert things, a client can check viaisSupported(source, target)
if the source-format and target-format is supported by this command (handler would by a common name, too)Mp3ToOggConversionCommand
Every conversion is a command, one can decide that this is too much, but with that you can save state in the command, like executation duration, error states, sizes, redo, undo, execute asynchronous...ConversionStrategy
with ImplEncoderConversionStrategy
encapsulates the concrete implementation of conversion, that is your duplicate code, this strategy how to convert 2 files by using the Encoder
Note that ConversionCommands should be prototypes, so that each execute happens in its own Command instance.
Now in code, you would setup the AudioFileFormatConverter and add the supported ConversionCommands to it.
Existing code
Obey the basic OO principles.
- DRY - Don't repeat yourself. If you find code is duplicate, introduce abstractions or extract methods.
- Use interfaces
In your case you can easily improve the code if you generalize the mp3ToOgg
and oggToMp3
methods.
So that you have only one general method for converting with the signature:
private void convert(File source, File target, String format, String codec);
-
\$\begingroup\$ What if I have other conversions? wavtOmp3, mp3ToWav, oggToWav, WavToOgg, what is the preferreable approach? \$\endgroup\$user962206– user9622062013年01月03日 04:13:02 +00:00Commented Jan 3, 2013 at 4:13
-
\$\begingroup\$ Also why would I use the Command Pattern? I can see the reason for chain of responsibility but not for command pattern \$\endgroup\$user962206– user9622062013年01月03日 04:46:31 +00:00Commented Jan 3, 2013 at 4:46
-
\$\begingroup\$ edited answer, chain and command relate a bit in this as you see \$\endgroup\$burna– burna2013年01月03日 07:08:05 +00:00Commented Jan 3, 2013 at 7:08
-
\$\begingroup\$ Also, I have implemented the Command Pattern in my case, please look at this also what do you mean by prototype codereview.stackexchange.com/questions/20114/… \$\endgroup\$user962206– user9622062013年01月03日 07:10:32 +00:00Commented Jan 3, 2013 at 7:10
Have you considered using a single method?
private void XToY(format,codec,source, target){
//ADD CODE FOR CHANGING THE EXTENSION OF THE FILE
encoAttrs.setFormat(format);
audioAttr.setCodec(codec);
encoAttrs.setAudioAttributes(audioAttr);
try{
encoder.encode(source, target, encoAttrs);
}catch(Exception e){
System.out.println("Encoding Failed");
}
}
You can theoretically use this one method to convert any source type to any target type.
-
\$\begingroup\$ I do not change the extension, I literraly convert the file. \$\endgroup\$user962206– user9622062013年01月03日 07:00:38 +00:00Commented Jan 3, 2013 at 7:00
-
\$\begingroup\$ I simply generalized your own code. \$\endgroup\$Captain Kenpachi– Captain Kenpachi2013年01月03日 12:58:50 +00:00Commented Jan 3, 2013 at 12:58
Explore related questions
See similar questions with these tags.