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.

The if/else statements below are not good. How can I improve this method?

public T GetContentByNodeIdSync<T>(Guid nodeId)
{
    var data = m_CMSCatalog.GetContentByNodeId(nodeId);

    if (typeof(T) == typeof(WebFolder))
    {
        var model = (WebFolderDTO)data;
        return Mapper.Map<WebFolderDTO, T>(model);
    }
    else if (typeof(T) == typeof(ContentListItem))
    {
        return Mapper.Map<ContentListItemDTO, T>((ContentListItemDTO)data);
    }
    else if (typeof(T) == typeof(Image))
    {
        return Mapper.Map<ImageDTO, T>((ImageDTO)data);
    }
    else if (typeof(T) == typeof(File))
    {
        return Mapper.Map<FileDTO, T>((FileDTO)data);
    }
    else if (typeof(T) == typeof(Folder))
    {
        return Mapper.Map<FolderDTO, T>((FolderDTO)data);
    }
    else if (typeof(T) == typeof(WebRoot))
    {
        return Mapper.Map<WebRootDTO, T>((WebRootDTO)data);
    }
    else if (typeof(T) == typeof(Article))
    {
        var model = (ArticleDTO)data;
        return Mapper.Map<ArticleDTO, T>(model);
    }
    else if (typeof(T) == typeof(WebContent))
    {
        var model = (WebContentDTO)data;
        return Mapper.Map<WebContentDTO, T>(model);
    }
    return default(T);
}
share|improve this question
1  
Simply don't do this; this is an abuse of generics. What you have here are ten methods crammed into one. Just write ten methods. –  Eric Lippert Aug 22 at 3:31

3 Answers 3

You should create a Dictionary<Type,Type> that would associate your object's type with their DTO equivalent, so you wouldn't need any if. Then you can use the dictionary to find the types to map. ex :

//This should be instanciated wherever you want, but try to do it only once. 
//The dictionary should be static
dictionary = new Dictionary<Type,Type>();
dictionary.Add(typeof(File),typeof(FileDTO));
//etc..

public T GetContentByNodeIdSync<T>(Guid nodeId)
{
    var data = m_CMSCatalog.GetContentByNodeId(nodeId);
    Type dtoType;
    bool mapExists = dictionary.TryGetValue(typeof(T), out dtoType); //To make sure you have an existing map in your dictionary.

    if(mapExists)
        return (T)Mapper.Map(data, dtoType, typeof(T));

    return default(T);
}
share|improve this answer

This method really shouldn't be generic, switching on typeof is generally a clue here. While generics are nice and this does allow the caller to know what object they'll get back, this is essentially a method that takes object and switches, which is not good. Can you think of a solution that better uses Polymorphism?

The easiest thing here would be to dump the generics and just use overloads, moving any shared code into a private method. But, you can't then call default(T) on all the other types when you use overloads, you will need to decide what objects these method applies to. I like overloading but this isn't the solution I would use.

//Overloading
public WebFolder GetContentByNodeIdSync(WebFolder folder, Guid nodeId)
public Image GetContentByNodeIdSync(Image image, Guid nodeId)
etc etc

However, I think the real problem here is that we're violating the Single Responsibility Principal. Chances are the Objects WebFolder, Image etc should encapsulate the logic for their respective GetContentByNodeIdSync, rather than lumping it together here. If it's not your class then inherit. Implement some interface with this method.

//Much better. SOLID, you might say.
public interface MyInterface
{
    public void/MyInterface GetContentByNodeIdSync(Guid nodeId);
}

MyInterface foo = new WebFolder();
var maybeNoOutput = foo.GetContentByNodeIdSync();
//Important question, are we creating more objects or just mutating this one? 
//(is it really a 'get'?, to me it looks like we're populating our object with the dto info)

While I''m banging on about the SRP what is this mapper helper class doing anyway? Maybe it would be better to put User-Defined Conversions in the classes themselves to cast them from their Data Transfer Objects and vice versa.

class WebFolder
{
    static public explicit operator WebFolder(WebFolderDTO folder)
    {
        return Convert From DTO
    }

    static public explicit operator WebFolderDTO (WebFolder folder)
    {
        return Convert To DTO
    }
}

var folder = new WebFolder();
folder.someProperty = "foo";
var folderDTO = (WebFolderDTO)folder;
var recoveredFolder = (WebFolder)folderDTO;
Assert.IsTrue(recoveredFolder.someProperty == "foo")
share|improve this answer
    
Thank for your suggestion. I think I will try to use Polymorphism. –  Tien Pham Aug 21 at 17:04
    
You can't overload on the return type of a method in C#. –  svick Aug 22 at 18:56
    
@svick correct. I didn't mean "<", I meant "(". Overloading isn't quite the right word, because they have different returns but it's a similar concept. –  Nathan Cooper Aug 22 at 21:01

You could get the name of the type and use a switch:

public T GetContentByNodeIdSync<T>(Guid nodeId) {
  var data = m_CMSCatalog.GetContentByNodeId(nodeId);
  switch (typeof(T).Name) {
    case "WebFolder": return Mapper.Map<WebFolderDTO, T>((WebFolderDTO)data);
    case "ContentListItem": return Mapper.Map<ContentListItemDTO, T>((ContentListItemDTO)data);
    case "Image": return Mapper.Map<ImageDTO, T>((ImageDTO)data);
    case "File": return Mapper.Map<FileDTO, T>((FileDTO)data);
    case "Folder": return Mapper.Map<FolderDTO, T>((FolderDTO)data);
    case "WebRoot": return Mapper.Map<WebRootDTO, T>((WebRootDTO)data);
    case "Article": return Mapper.Map<ArticleDTO, T>((ArticleDTO)data);
    case "WebContent": return Mapper.Map<WebContentDTO, T>((WebContentDTO)data);
    default: return default(T);
  }
}
share|improve this answer
3  
You will lose type safety though, so if you would refactor a class which is checked in a the switch case, the code will compile but you'd get an runtime exception. The if statements will create a compiler error. –  derape Aug 21 at 12:20
    
As somebody having to actively correct this bug right now in production code, no. Just, no. –  insta Aug 21 at 14:38
    
I dislike multiple statements in one line :( –  DebugErr Aug 21 at 17:49
1  
@DebugErr: There are no multiple statements. Do you mean that you dislike having the case and the code for it on the same line? That is simply an opinion of formatting, you can format your own code any way you like. –  Guffa Aug 21 at 18:22
1  
-1 - this breaks type safety, and there's no oint having a generic constraint if you are switching on T. –  Dan Pantry Aug 21 at 20:25

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.