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 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?

share|improve this question
add comment

2 Answers

up vote 5 down vote accepted

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

  • AudioFormatConverters 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 to Files, and holds a list of supported ConversionCommands
  • ConversionCommand encapsulates logic to convert things, a client can check via isSupported(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 Impl EncoderConversionStrategy 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);
share|improve this answer
    
What if I have other conversions? wavtOmp3, mp3ToWav, oggToWav, WavToOgg, what is the preferreable approach? –  user962206 Jan 3 '13 at 4:13
    
Also why would I use the Command Pattern? I can see the reason for chain of responsibility but not for command pattern –  user962206 Jan 3 '13 at 4:46
    
edited answer, chain and command relate a bit in this as you see –  burna Jan 3 '13 at 7:08
    
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/… –  user962206 Jan 3 '13 at 7:10
add comment

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.

share|improve this answer
    
I do not change the extension, I literraly convert the file. –  user962206 Jan 3 '13 at 7:00
    
I simply generalized your own code. –  Juann Strauss Jan 3 '13 at 12:58
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.