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开发者_Go百科();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.
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;
}
}
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.
The code is cool.
- you check for arguments before get the work started.
- your code is clear and easy to read
- 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.
精彩评论