current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
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.
up vote 3 down vote favorite
1

I copy addresses from the endpoints to an array that should only hold the endpoint addresses. This works but it seems so old school :/

Can I make this faster, better, more sexy (LINQ)? 

public static EndpointAddress[] Find()
{
    EndpointAddress[] endpointAddresses = null;
    var discoveryClient = new DiscoveryClient(new UdpDiscoveryEndpoint());
    var findResponse = discoveryClient.Find(
        new FindCriteria(
            typeof(ICommunicationService)));

    if(findResponse != null) 
    {
        if(findResponse.Endpoints.Count > 0) 
        {
            endpointAddresses = new EndpointAddress[findResponse.Endpoints.Count];

            for (int i = 0; i <= findResponse.Endpoints.Count; i++)
            {
                endpointAddresses[i] = findResponse.Endpoints[i].Address;
            }
        }
    }

    return endpointAddresses ?? new EndpointAddress[0];
}
share|improve this question
2  
Does this code even work? i <= findResponse.Endpoints.Count should cause an index out of range error. –  CodesInChaos Jun 29 at 13:08
    
It isn't worth an answer of its own, but I don't think you should use var for findResponse since it is impossible to figure what is the type just by looking at the code! –  TopinFrassi Jun 29 at 19:49

4 Answers 4

up vote 10 down vote accepted

You certainly can! You're currently coding using the "arrow pattern" it's often nicer to return early rather than keep indenting further and further as you nest conditionals.

if (findResponse == null) 
{
     return new EndpointAddress[0];
}

Now let's make the rest of the method shorter:

public static EndpointAddress[] Find()
{
    var discoveryClient = new DiscoveryClient(new UdpDiscoveryEndpoint());
    var findResponse = discoveryClient.Find(
        new FindCriteria(
            typeof(ICommunicationService)));

    if(findResponse == null) 
    {
        return new EndpointAddress[0];
    }
    return findResponse.Endpoints.Select(endpoint => endpoint.Address).ToArray();
}

But, we can go further. You should prefer returning interfaces rather than concrete representations. Do you need indexed access to the EndpointAddresses? I doubt it, return IEnumerable<EndpointAddress> instead:

public static IEnumerable<EndpointAddress> Find()
{
    var discoveryClient = new DiscoveryClient(new UdpDiscoveryEndpoint());
    var findResponse = discoveryClient.Find(
        new FindCriteria(
            typeof(ICommunicationService)));

    if(findResponse == null) 
    {
        return Enumerable.Empty<EndpointAddress>();
    }
    return findResponse.Endpoints.Select(endpoint => endpoint.Address);
}

You could also inject the DiscoveryClient into the classes constructor but as it is a static method and I don't know what the rest of the class looks like, I'll leave that point for now.

share|improve this answer
    
I like this version! Nice explanation! Thank you :) –  SeveSeve Jun 29 at 11:18
    
What do you think of "return findResponse == null ? Enumerable.Empty<EndpointAddress>() : findResponse.Endpoints.Select(endpoint => endpoint.Address);" ? –  SeveSeve Jun 29 at 13:41
    
@SeveSeve A good change. Getting too used to ReSharper telling me to do things like that to spot them myself! –  RobH Jun 29 at 13:43
    
If you return IEnumerable<EndpointAddress>, your query will be lazy evaluated, I don't know avec the return type of findResponse, but if it happens to be a lazy evaluated IEnumerable<> too, you could have a problem. Simply adding .ToList() at the end of the return would evaluate the expression, and you can still return IEnumerable<> –  TopinFrassi Jun 29 at 19:46
    
@TopinFrassi - Endpoints is a Collection<T> which is not lazily evaluated. –  RobH Jun 30 at 7:25
up vote 7 down vote

If findResponse.EndPoints is an IEnumerable, this one line should work for you:

endpointAddresses = findResponse.Endpoints.Select(endpoint => endpoint.Address).ToArray();
share|improve this answer
1  
Following this line: findResponse.Endpoints.Count, you can assume that this is a List<Endpoint>. And since a List<T> inherits IEnumerable<T>, your line will work. :) –  Abbas Jun 29 at 10:26
    
@Abbas, you can't assume that its a List<T> just because it has a Count property. –  Ash Burlaczenko Jun 29 at 10:46
    
Since it has that property, I assume it is some form of List<T> or similar to that because in .NET, by my knowledge the only collections that have this property. And since. It is the result of some sort of webservice call, I'll assume it is a List<T> or similar. –  Abbas Jun 29 at 10:52
    
It's actually Collection<T> but having indexed acess and a count property is a good indication of ICollection<T> (and IEnumerable<T>) but it's no guarantee. –  RobH Jun 29 at 10:56
1  
@Abbas, Yes a lot of the time it's going to be a collection but I'm trying to get across the point that you can't assume a type base on knowing property. In this case Endpoints could quite easily be a custom class which has a Count property. –  Ash Burlaczenko Jun 29 at 12:05
up vote 2 down vote

This is how you can rewrite the code using linq

    public static EndpointAddress[] Find()
    {
        List<EndpointAddress> endpointAddresses = new List<EndpointAddress>();
        var discoveryClient = new DiscoveryClient(new UdpDiscoveryEndpoint());
        var findResponse = discoveryClient.Find(
            new FindCriteria(
                typeof(ICommunicationService)));

        if (findResponse != null && findResponse.Endpoints.Any())
        {
            endpointAddresses.AddRange(findResponse.Endpoints.Select(metadata => metadata.Address));
        }

        return endpointAddresses.ToArray();
    }

In the above code I have used findResponse.Endpoints.Any instead of using findResponse.Endpoints.Count. Then linq query is used to fetch the address. I have used a List for endpointAddresses as you have initialize it multiple times if its an array. I am not sure if findResponse is null. If it is never null then you can avoid the null check.

share|improve this answer
2  
Please add an explanation to your code, code-only answers are not recommended on CodeReview. –  Abbas Jun 29 at 10:27
up vote 2 down vote

While I would probably use LINQ, just because I like it, you don't have to. In order to make the code a bit less clunky, just remove the parts that are not needed.

public static EndpointAddress[] Find()
{
    var discoveryClient = new DiscoveryClient(new UdpDiscoveryEndpoint());
    var findResponse = discoveryClient.Find(new FindCriteria(typeof(ICommunicationService)));

    int numberOfEndpoints = findResponse != null ? findResponse.Endpoints.Count : 0;
    var endpointAddresses = new EndpointAddress[numberOfEndpoints];
    for (int i = 0; i < numberOfEndpoints; i++)
    {
        endpointAddresses[i] = findResponse.Endpoints[i].Address;
    }

    return endpointAddresses;
}

The check for null is really only needed to determine the size of the output array. The check for emptiness isn't needed at all.

share|improve this answer

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.