Improving a method to guarantee uniqueness

I have just written something to guarantee a unique URL when creating pages within an old legacy system (sounds odd doing this retrospectively but we’ve just changed how the routing works). I don’t often end up using while and I think this may be why I feel it can be improved.

private string GetUniqueUrl(Page parentPage, Page newPage)
{
    var isNotDuplicatateUrl = false;
    var version = 0;
    var baseUrl = parentPage != null ? parentPage.FullUrl.Trim() + "/" : string.Empty;

    while (isNotDuplicatateUrl == false)
    {
        var friendlyUrl = version > 0 ? (newPage.FriendlyURLName + "-" + version) : newPage.FriendlyURLName;
        var fullUrl = baseUrl + friendlyUrl;

        var duplicateUrlPage =
            Site.Pages.FirstOrDefault(x => x.FullUrl != null && string.Equals(x.FullUrl.Trim(), fullUrl.Trim(), StringComparison.CurrentCultureIgnoreCase));

        if (duplicateUrlPage == null)
        {
            isNotDuplicatateUrl = true;
        }
        else
        {
            version++;
        }
    }

    return newPage.FriendlyURLName + "-" + version;
}

NB: I know Page.FriendlyURLName should be Page.FriendlyUrlName but I’m only looking for improvements within the scope of this method.

Having a loop to check each version of the URL could become a problem if there end up being a lot of URL’s. There many be a better way to do it using a hash, or some other means of persisting data, and retrieving the most recently used version.

Still, even with your loop code, there’s a way to do it in a simpler fashion, using a helper method to check for a duplicate URL is a start…..

private bool PageFound(string baseUrl, string friendlyURL)
{
    var fullUrl = baseUrl + friendlyUrl;
    var duplicateUrlPage = Site.Pages.FirstOrDefault(
                 x => x.FullUrl != null
              && string.Equals(x.FullUrl.Trim(), fullUrl.Trim(),
                       StringComparison.CurrentCultureIgnoreCase));
    return duplicateUrlPage != null;
}

private string GetUniqueUrl(Page parentPage, Page newPage)
{
    var version = 0;
    var baseUrl = parentPage != null ? parentPage.FullUrl.Trim() + "/" : string.Empty;
    var friendlyUrl = newPage.FriendlyURLName;
    while (PageFound(baseUrl, friendlyURL))
    {
        version++;
        friendlyUrl = newPage.friendlyURLName + "-" + version;
    }
    return newPage.FriendlyURLName + "-" + version;
}

There are two things to pay attention to here.

1. Note how the first time the duplicate is checked, there’s no ‘version’ on the URL? The version only gets added for version >=1. This is ‘managed’ by doing the first URL construction outside the loop, and the rest inside the loop. This saves the conditional on the version == 0
2. I believe there’s a bug in your code (which I have reproduced in my suggestion). If you search for the page without the version (version = 0) and you do not find one, you return the URL with the version anyway (you append “-0”). Are you sure this is what you want to do? If it is not what you want, you can just return friendlyUrl instead of newPage.FriendlyURLName + "-" + version

Using a helper function to check for the duplicate is a good way to put a code block in to a conditional clause for a loop. In essence, by creating a function, we have also created a powerful while loop check. The ‘problem’ with your code is that it was trying to do condition-loop logic inside the execution block, instead of inside the loop clause
2.

For each iteration of the while loop you are calling

    var duplicateUrlPage =
        Site.Pages.FirstOrDefault(x => x.FullUrl != null &&   
            string.Equals(x.FullUrl.Trim(), fullUrl.Trim(), StringComparison.CurrentCultureIgnoreCase));

and therefor you are calling fullUrl.Trim() for each Page in Pages for each iteration of the while loop.

You should declare a IEnumerable<String> outside of the while loop and fill it like

IEnumerable<String> pages = Site.Pages
           .Where(p=> !String.IsNullOrEmpty(p))
           .Select(p=> p.FullUrl.Trim());  

while (true)
{
    var friendlyUrl = version > 0 ? (newPage.FriendlyURLName + "-" + version) : newPage.FriendlyURLName;
    var fullUrl = (baseUrl + friendlyUrl).Trim();

    if (!pages.Any(x => string.Equals(x, fullUrl, StringComparison.CurrentCultureIgnoreCase)))
    {
        return friendlyUrl; 
    }
    version++;
}  

also as you don’t need the duplicateUrlPage you should use Any() instead of FirstOrDefault().

What I don’t like is the execution of the tenary expression on every iteration. So let us declare friendlyUrl outside of the loop.

IEnumerable<String> pages = Site.Pages
           .Where(p=> !String.IsNullOrEmpty(p))
           .Select(p=> p.FullUrl.Trim());  
var friendlyUrl = newPage.FriendlyURLName;
while (true)
{
    var fullUrl = (baseUrl + friendlyUrl).Trim();

    if (!pages.Any(x => string.Equals(x, fullUrl, StringComparison.CurrentCultureIgnoreCase)))
    {
        return friendlyUrl; 
    }
    version++;
    friendlyUrl = newPage.FriendlyURLName + "-" + version;
}