2
\$\begingroup\$

I want to change the task edit form of the workflow when an approver wants to approve/reject a task. Please review and suggest improvements or any other possible solution with an explanation.

 public void createTask1_MethodInvoking(object sender, EventArgs e)
        {
            createTask1_TaskId1 = Guid.NewGuid();
            createTask1_TaskProperties1.Title = "Test Title";
            createTask1_TaskProperties1.AssignedTo = "i:0#.w|directory\approver";
            var taskList = workflowProperties.TaskList;                
            var wfContentType = taskList.ContentTypes["Workflow Task"];
            TaskTitle = workflowProperties.Item["Title"].ToString();
            wfContentType.DisplayFormUrl = "_Layouts/SharePointProject4/ApplicationPage1.aspx";
            wfContentType.EditFormUrl = wfContentType.DisplayFormUrl;
            wfContentType.Update();
            taskList.NavigateForFormsPages = false;
            taskList.Update();
        }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ What would trigger the above code to run (e.g. a list event receiver, feature receiver, custom form code, etc)? Also, can you clarify whether you want to change the form for a specific task or for all workflow tasks in the task list? \$\endgroup\$
    – Thriggle
    Commented May 5, 2015 at 15:07

1 Answer 1

4
\$\begingroup\$

Give your code some room to breathe.

Empty space costs nothing in C#, so add some gaps to group together similar concepts.

 public void createTask1_MethodInvoking(object sender, EventArgs e)
    {
        createTask1_TaskId1 = Guid.NewGuid();
        createTask1_TaskProperties1.Title = "Test Title";
        createTask1_TaskProperties1.AssignedTo = "i:0#.w|directory\approver";

        var taskList = workflowProperties.TaskList;                
        var wfContentType = taskList.ContentTypes["Workflow Task"];

        TaskTitle = workflowProperties.Item["Title"].ToString();

        wfContentType.DisplayFormUrl = "_Layouts/SharePointProject4/ApplicationPage1.aspx";
        wfContentType.EditFormUrl = wfContentType.DisplayFormUrl;
        wfContentType.Update();

        taskList.NavigateForFormsPages = false;
        taskList.Update();
    }

Secondly, when naming variables, be explicit. There's no need to abbreviate anything, once again, extra characters are free.

  public void createTask1_MethodInvoking(object sender, EventArgs e)
    {
        createTask1_TaskId1 = Guid.NewGuid();
        createTask1_TaskProperties1.Title = "Test Title";
        createTask1_TaskProperties1.AssignedTo = "i:0#.w|directory\approver";

        var taskList = workflowProperties.TaskList;                
        var workFlowContentType = taskList.ContentTypes["Workflow Task"];

        TaskTitle = workflowProperties.Item["Title"].ToString();

        workFlowContentType.DisplayFormUrl = "_Layouts/SharePointProject4/ApplicationPage1.aspx";
        workFlowContentType.EditFormUrl = wfContentType.DisplayFormUrl;
        workFlowContentType.Update();

        taskList.NavigateForFormsPages = false;
        taskList.Update();
    }

Next, onto design. You've named a task createTask1, which is concerning, because that means it's possible you've created a lot of these with different numbers. Pop them in a collection named createTasks instead and try to refactor your MethodInvoking event handler to be abstracted across all createTasks. If there's something about createTask1 that means the event handler logic cannot be applied to the other createTask instances, then you should name createTask1 based on this unique property.

It's hard to tell what this method handler is meant to be doing since it's just named after the event that calls it. Give it a better name, or extract this functionality into its own method with a sensible name and call it from createTask1_MethodInvoking.

Lastly, I'm seeing an absolute ton of magic strings, which make it very hard indeed to sensibly refactor, because you'll have to change things in so many places, and they also hamper the understandability of the code. Make pretty much all of those strings into constant fields with sensible names.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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