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.

This is what I have, But I think I can write it in a sohrter and more efficient way too, what do you suggest?

    string actionString= string.Empty;

    if (auditAction == DSRHelper.SaveStatusEnum.Save)
    {
        actionString = Properties.Resources.SAVE_ACTION_MESSAGE;
    }

    if (auditAction == DSRHelper.SaveStatusEnum.Complete && model.Dirty) 
    {
        actionString = "Save'NComplete";
    }
    else if(auditAction == DSRHelper.SaveStatusEnum.Complete)
    {
        actionString = "OnlyComplete.";
    }

EDIT: Ok I changed it a little bit, better now?

    string actionString= string.Empty;

    if (auditAction == DSRHelper.SaveStatusEnum.Save)
    {
        actionString = Properties.Resources.SAVE_ACTION_MESSAGE;
    }
    else if (auditAction == DSRHelper.SaveStatusEnum.Complete ) 
    {
        if (model.Dirty)
        {
            actionString = "Save'NComplete";
        }
        else
        {
            actionString = "OnlyComplete";
        }
    }
share|improve this question
1  
I really don't like the nested 'if' in the 'else if'. I would go with the method solution below. –  Jeff Vanzella Nov 8 '12 at 22:37
    
I'm not very fond of switches... how about a stack ternary operators? XD –  Omega Nov 8 '12 at 23:09
1  
As @Omega suggested perhaps if not the switch then you could also look at the ternary I used for dirty part i.e. actionString = model.IsDirty ? "Save'NComplete" : "OnlyComplete" to further shorten it if you wanted –  dreza Nov 9 '12 at 1:54
    
I am not that fond of enums, they always mess the code with if's, switches. Can't you specialize your class and move this logic in each specialized class? –  whoisthis Nov 9 '12 at 6:56

1 Answer 1

up vote 7 down vote accepted

Maybe a switch? Although whether it's shorter or more efficient or even easier to read up for debate.

string actionString = GetStatusMessage(auditAction);

// ...
function GetStatusMessage(DSRHelper.SaveStatusEnum status) 
{
   switch(status)
   {
      case DSRHelper.SaveStatusEnum.Save:
           return Properties.Resources.SAVE_ACTION_MESSAGE;
      case DSRHelper.SaveStatusEnum.Complete:
           return model.Dirty ? "Save'NComplete" : "OnlyComplete.";
      default:
           return string.Empty;
   }
}
share|improve this answer
1  
I was 80% typing this exact same answer. One thing though, you might have to pass in the model object to the GetStatusMessage method. –  Jeff Vanzella Nov 8 '12 at 22:36
    
@JeffVanzella yeah I thought about that after I wrote it and then figured I'd wait until the OP stated that it wasn't a class level variable. Just another way to skin this cat :) –  dreza Nov 9 '12 at 1:52

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.