2
\$\begingroup\$

I have the following code:

public EmployeeProcessOutput  RetrieveEmployee(EmployeeProcessInput input)
{     
    var employee = input.Employee;

    if (input.EmployeeId == null)
        employee= CreateEmployee(employee);

    return new EmployeeProcessOutput
    {
        Employee = employee,
        State = "Stored" 
    };
}

private Employee CreateEmployee(employee)
{
    //Call a service to create an employee in the system using the employee info 
    //such as name, address etc and will generate an id and other employee info.
    var output = Service(employee)
    var employee = output.Employee;
    return employee;
}

Is there a clean way and of passing the employee parameter to the CreateEmployee method? I feel that this line could be cleaner:

employee = CreateEmployee(employee);

Any thoughts?

\$\endgroup\$
  • \$\begingroup\$ What don't you like about it wizkid? \$\endgroup\$ – dreza Feb 19 '14 at 6:15
  • 1
    \$\begingroup\$ Next time, could you please make sure to include code that actually compiles? \$\endgroup\$ – svick Feb 19 '14 at 11:31
  • \$\begingroup\$ Small note, possible failure point, you are checking if input.Employee == null but if input is null your app will crash, i'd suggest performing a if(input == null) throw new NullArgumentException("input") to assist in debugging later. \$\endgroup\$ – apieceoffruit Feb 19 '14 at 12:16
3
\$\begingroup\$

Have a look at this. I think the real question is pass by value vs pass by reference.

More info here.

Edit: The line could be modified to following:

CreateEmployee(ref employee);

and the method name would be:

private void CreateEmployee(ref employee)
\$\endgroup\$
  • 3
    \$\begingroup\$ Could you add anything from these links? Link-only answers are discouraged here. \$\endgroup\$ – Jamal Feb 19 '14 at 5:27
  • \$\begingroup\$ I'm not familiar with c#. But taking that its a OOP originated from c, I had given the answer. \$\endgroup\$ – bikram990 Feb 19 '14 at 5:38
  • \$\begingroup\$ objects are always passed by reference. "ref" is redundant here \$\endgroup\$ – wizzardz Feb 19 '14 at 6:42
  • 2
    \$\begingroup\$ @wizzards No, it's not redundant. For reference types, the reference is passed by value, unless you use ref. And since the reference changes here, that ref is necessary. \$\endgroup\$ – svick Feb 19 '14 at 11:49
  • 1
    \$\begingroup\$ @wizzardz: You are suffering from a common misconception which even some prominent Microsoft developers (those who participated in Framework Design Guidelines) suffer from. Svick is totally correct in pointing this out. \$\endgroup\$ – Magus Feb 19 '14 at 17:57
1
\$\begingroup\$

Well, your code is really confusing. Here is what i think what your signatures should look like:

public bool StoreEmployee(Employee employee)
{
    //here you set ID, etc. and store eployee into your system
    //the return value indicates whether operaration succeeded
    //alternatively you can return void and throw an exception in case of error
}

public Employee GetEmployee(string id)
{
    //here you fetch employee by id from your system
    //returns null or throws an exception if employee was not found
}


public Employee GetEmployee(EmployeeName name)
{
    //here you fetch employee by name from your system
    //where EmployeeName contains required info (first and last names?)
    //returns null or throws an exception if employee was not found
}

I am not sure where your input/output classes are coming from, but they feel really outdated. This is definetely NOT how you want to design a C# application. If you absolutely must return a state of operation - use method return value for that. And use bool or enums for that, not strings. But it is not something to build your application around when you are using an OOP language.

\$\endgroup\$
0
\$\begingroup\$

Rather than finding a better way for calling method you need to re-factor your code. Depending on the situation you may create a new class which will provide you a fresh employee. But if this is a single task and need only once extract method can be a good solution. I've just extracted some code from your RetriveEmployee method into two methods.

public EmployeeProcessOutput RetrieveEmployee(EmployeeProcessInput input)
{
    return new EmployeeProcessOutput
               {
                   Employee = GetFreshEmployee(input),
                   State = "Stored" 
               };
}

private Employee GetFreshEmployee(EmployeeProcessInput imput)
{
    if (HasValidEmployee(input))
    {
        return input.Employee
    }
    else
    {
        return CreateEmployee(input.Employee)
    }
}

private bool HasValidEmployee(EmployeeProcessInput input)
{
    return input.EmployeeId != null;
}

Hopefully it is easier to understand then the original implementation. If you prefer ternary for single line conditional branching use it in GetFreshEmployee which will concise your code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.