CODEWITHSUNDEEP
c#exceptionconstructor
Sergey
Sergey

Reputation: 105

Way to create an object

I have a constructor

public Track(string path)
{
        if (!File.Exists(path))
            throw new FileNotFoundException("File not found", path);
        if (!IsAudioFile(path))
            throw new Exception("Illegal Audio Format");

        _path = path;
        _id = Guid.NewGuid();
        _rate = 0;
        _length = GetTrackLength(path);

        TagLib.File file = TagLib.File.Create(path);
        if (!file.Tag.IsEmpty)
        {
            try
            {
                _artist = file.Tag.Artists[0];
            }
            catch (Exception e)
            {
                _artist = "";
            }
            _title = file.Tag.Title;
            try
            {
                _genre = file.Tag.Genres[0].ToGenre();
            }
            catch (Exception e)
            {
                _genre = Genre.NoGenre;
            }
        }
        else
        {
            _artist = "Unknown";
            _title = "Unknown";
            _genre = Genre.NoGenre;
        }
 }

Should I throw an exceptions or i should choose another way of creating object? For example:

Track track = new Track(path);
track = Track.GetInstance();

Upvotes: 1

Views: 190

Answers (4)

Chris S
Chris S

Reputation: 65426

I'd make some minor amendments as below, to avoid unnecessary exception handling, and also throw a more specalised exception in the argument checking. As far as throwing in constructors is concerned, that is fine as it is really just special type of method.

Concerning feedback of creating a new Track: You could place a GetTrack() method in some kind of manager (TagManager for example), if you think new'ing up a Track object is something that your API should handle instead of giving the responsibility to the consumer.

public Track(string path)
{
    if (!File.Exists(path))
        throw new FileNotFoundException("File not found", path);
    if (!IsAudioFile(path))
        throw new InvalidOperationException("Illegal Audio Format");

    _path = path;
    _id = Guid.NewGuid();
    _rate = 0;
    _length = GetTrackLength(path);

    TagLib.File file = TagLib.File.Create(path);
    if (!file.Tag.IsEmpty)
    {
        _title = file.Tag.Title;

        if (file.Tag.Artists != null && file.Tag.Artists.Count > 0)
            _artist = file.Tag.Artists[0];
        else
            _artist = "";


        if (file.Tag.Genres != null && file.Tag.Genres.Count > 0)
            _genre = file.Tag.Genres[0].ToGenre();
        else
            _genre = Genre.NoGenre;
    }
    else
    {
        _artist = "Unknown";
        _title = "Unknown";
        _genre = Genre.NoGenre;
    }
}

Upvotes: 0

Vladimir Ivanov
Vladimir Ivanov

Reputation: 43098

The code is cool.

  1. you check for arguments before get the work started.
  2. your code is clear and easy to read
  3. you handle the situation when no info is provided setting the default value.

All I can advice is not to use generic Exception, but throw and catch something more specific to context. Feel free to use ArgumentException.

Also, just for checkstyle:

It is a good practice to use curly braces {} always with if cause if you want to add the code in if { } block, you can forget to put the braces. Though it is not necessary here.

Upvotes: 0

Donnie
Donnie

Reputation: 46913

There is (generally) no reason to have a static method return an instance unless you're doing a singleton pattern or need to otherwise re-use existing objects instead of creating new instances.

Throwing exceptions in a constructor is fine, and generally the right way of doing things.

Upvotes: 0

SLaks
SLaks

Reputation: 887415

Your code is correct and well-patterned.

However, you shouldn't throw the base Exception class.
Instead, you should throw an ArgumentException, InvalidDataException, or InvalidOperationException.

You also shouldn't catch the base Exception class.
Instead, you should catch whatever exceptions the the ToGenre method can throw.

Upvotes: 1

Related Questions