3
\$\begingroup\$

Is this good way to create a get API method with different service methods? Based on the scope parameter, it decides what service method in the API to call.

Enum:

    enum scopeValue
    {
          None,
          GetAll, 
          GetWithDetails 
    };

API:

   public class WorkItemController : BaseController
    {
        private IWorkItemService workItemService;

        public WorkItemController(IWorkItemService workItemService)
        {
            this.workItemService = workItemService;
        }

        [HttpGet]
        public async Task<IEnumerable<WorkItemDto>> Get(int id, string scope = "None")
        {
            scopeValue choice;

            Enum.TryParse(scope, out choice);

            if (choice == scopeValue.None)
            {
                return await workItemService.Get(id);
            }
            else if (choice == scopeValue.GetAll)
            {
                return await workItemService.GetAll();
            }
            else if (choice == scopeValue.GetWithDetails)
            {
                return await workItemService.GetAllWithDetails();
            }
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ This is real code in progress. I am still not sure about design api so I am asking if this is good aproach. But its working. \$\endgroup\$ – Raskolnikov Nov 27 '15 at 9:36
4
\$\begingroup\$

Let's think about what your possible URIs would look like.

http://example.com/WorkItem?id=5
http://example.com/WorkItem?id=5&scope=GetAll

That's not how I'd expect the API to work. Typically, calling the resource without an id will return all of them.

// returns all work items
http://example.com/WorkItem

// returns work item by id
http://example.com/WorkItem/5

And then, to get the details for an item, we add an action to the end of the URI.

// returns details for specified item
http://example.com/WorkItem/details/5

To get the last part, you'll need to wire up a custom route, but it's not that difficult to do. You can then overload the Get methods to implement this. It's a much cleaner API to work with and, honestly, results in a much cleaner & maintainable controller.


I take it back, you won't need a custom route. MVC comes "out of the box" with a {controller}/{action}/{id} route, so for the Detail route, all you need to do is create the proper method.

[HttpGet]
public async Task<IEnumerable<WorkItemDto>> Details()

And

[HttpGet]
public async Task<IEnumerable<WorkItemDto>> Details(int id)

Will give you routes that look like this.

http://example.com/WorkItem/Details
http://example.com/WorkItem/Details/5
\$\endgroup\$
  • \$\begingroup\$ Updated my answer. You shouldn't even need a custom route. \$\endgroup\$ – RubberDuck Nov 27 '15 at 12:45
  • \$\begingroup\$ So {action} is name of api method ? Can I also call 'h ttp://example.com/WorkItem/detail' ? So without Id and with action. If I want to have 2 get methods without paramaters \$\endgroup\$ – Raskolnikov Nov 27 '15 at 12:52
  • \$\begingroup\$ Yes, but you'll need to specify a custom route. asp.net/mvc/overview/older-versions-1/controllers-and-routing/… \$\endgroup\$ – RubberDuck Nov 27 '15 at 12:55
  • \$\begingroup\$ Actually, you won't need a custom route. I screwed the order up. I fixed my answer. \$\endgroup\$ – RubberDuck Nov 27 '15 at 13:00
3
\$\begingroup\$

I this good way to...

Short answer: no

Longer answer

One of the reasons is clearly that a user will have pass some value for id although it wouldn't be needed e.g if the passed in scope != "None".

Another thing is, that you never really check the result of Enum.TryParse() which returns a bool for a purpose. If this public method is called by passing e.g "none" or null nothing is done without comunicating the reason to the caller like e.g throwing an exception.

If you still want to go this way, I would like to suggest that you check for null of scope and also use this overloaded Enum.TryParse(string, bool, out TEnum) method which adds the ability to ignore the case of the passed in string.

The very minimum should be to return early if Enum.TryParse() returns false.

\$\endgroup\$
  • \$\begingroup\$ is it pissible to do this wothout tryParse? Send enum insted of string. \$\endgroup\$ – Raskolnikov Nov 27 '15 at 11:43
2
\$\begingroup\$

I would refactor Get method and have something as below. This will look more cleaner and easier to maintain.

// This makes more sense to me. Because API states that 
// caller will get one object of WorkItemDto if it provides the ID.
public async Task<WorkItemDto> Get(int id)
{
}

public async Task<IEnumerable<WorkItemDto>> GetAll()
{
}

// so on ..
\$\endgroup\$
  • \$\begingroup\$ but I still need GetAl()l and GetAllWithDetails(). So I must have some enum as parameter of second function. \$\endgroup\$ – Raskolnikov Nov 27 '15 at 11:46

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.