Skip to main content
Spelling and grammar
Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

Performance is not the only important thing about a piece of code. Depending on the nature of the problem you might also need to consider - readability, maintainability and flexibility. I will present you couple of solutions focusing on those different aspects.

LINQ

You could use LINQ and write it like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    foreach (var item in source)
    {
        if (source.Count(t => t == item) > 1)
        {
            return item;
        }
    }
    return -1;
}

If you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    return source.FirstOrDefault(item => source.Count(t => t == item) > 1);
}

LINQ is usually quite readable and most C# programmers are familiar with it'sits syntax so it's a lot easier to just give it a glance and know what's up.

HashSet<>

The HashSet<T> is very handy when working with unique numbers as it can contain only distinct numbers. It's bool Add(T item) method will allow us to quickly check if a value was already added:

private static int FirstDuplicate(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    foreach (var item in source)
    {
        if (!distinctItems.Add(item))
        {
            return item;
        }
    }
    return -1;
}

Again, if you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate<int>(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    return source.FirstOrDefault(item => !distinctItems.Add(item));
}

Your code

It's often times easy to overthink a simple problem, such is your case, you've over-complicated a simple task. You should think through the problem, before writing code. Some people prefer to write first and than improve afterwards,afterwards; this also works, but it's always good to think a bit about it before-hand, especially. Especially when you're beginner you might often find yourself tunnel-visioned in a specific way of solving the problem once you're done with it,it; you could avoid that by thinking before-hand as you wontwon't be so attached to specific ideas.

Missing for loop indentation is also reducing the readability of your function.

Performance is not the only important thing about a piece of code. Depending on the nature of the problem you might also need to consider - readability, maintainability and flexibility. I will present you couple of solutions focusing on those different aspects.

LINQ

You could use LINQ and write it like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    foreach (var item in source)
    {
        if (source.Count(t => t == item) > 1)
        {
            return item;
        }
    }
    return -1;
}

If you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    return source.FirstOrDefault(item => source.Count(t => t == item) > 1);
}

LINQ is usually quite readable and most C# programmers are familiar with it's syntax so it's a lot easier to just give it a glance and know what's up.

HashSet<>

The HashSet<T> is very handy when working with unique numbers as it can contain only distinct numbers. It's bool Add(T item) method will allow us to quickly check if a value was already added:

private static int FirstDuplicate(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    foreach (var item in source)
    {
        if (!distinctItems.Add(item))
        {
            return item;
        }
    }
    return -1;
}

Again, if you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate<int>(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    return source.FirstOrDefault(item => !distinctItems.Add(item));
}

Your code

It's often times easy to overthink a simple problem, such is your case, you've over-complicated a simple task. You should think through the problem, before writing code. Some people prefer to write first and than improve afterwards, this also works, but it's always good to think a bit about it before-hand, especially when you're beginner you might often find yourself tunnel-visioned in a specific way of solving the problem once you're done with it, you could avoid that by thinking before-hand as you wont be so attached to specific ideas.

Missing for loop indentation is also reducing the readability of your function.

Performance is not the only important thing about a piece of code. Depending on the nature of the problem you might also need to consider - readability, maintainability and flexibility. I will present you couple of solutions focusing on those different aspects.

LINQ

You could use LINQ and write it like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    foreach (var item in source)
    {
        if (source.Count(t => t == item) > 1)
        {
            return item;
        }
    }
    return -1;
}

If you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    return source.FirstOrDefault(item => source.Count(t => t == item) > 1);
}

LINQ is usually quite readable and most C# programmers are familiar with its syntax so it's a lot easier to just give it a glance and know what's up.

HashSet<>

The HashSet<T> is very handy when working with unique numbers as it can contain only distinct numbers. It's bool Add(T item) method will allow us to quickly check if a value was already added:

private static int FirstDuplicate(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    foreach (var item in source)
    {
        if (!distinctItems.Add(item))
        {
            return item;
        }
    }
    return -1;
}

Again, if you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate<int>(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    return source.FirstOrDefault(item => !distinctItems.Add(item));
}

Your code

It's often times easy to overthink a simple problem, such is your case, you've over-complicated a simple task. You should think through the problem, before writing code. Some people prefer to write first and than improve afterwards; this also works, but it's always good to think a bit about it before-hand. Especially when you're beginner you might often find yourself tunnel-visioned in a specific way of solving the problem once you're done with it; you could avoid that by thinking before-hand as you won't be so attached to specific ideas.

Missing for loop indentation is also reducing the readability of your function.

deleted 522 characters in body
Source Link
Denis
  • 8.6k
  • 5
  • 33
  • 76

LINQ - readability

For loops - speed

private static int FirstDuplicate2(int[] a)
{
    for (int i = 0; i < a.Length - 1; i++)
    {
        for (int j = i + 1; j < a.Length; j++)
        {
            if (a[i] == a[j])
            {
                return a[i];
            }                   
        }
    }
    return -1;
}

HashSet<> - middle-ground

It's often times easy to overthink a simple problem, such is your case, you've over-complicated a simple task. As you can see my for loop is a lot shorter, simpler, faster and easier to understand. You should think through the problem, before writing code. Some people prefer to write first and than improve afterwards, this also works, but it's always good to think a bit about it before-hand, especially when you're beginner you might often find yourself tunnel-visioned in a specific way of solving the problem once you're done with it, you could avoid that by thinking before-hand as you wont be so attached to specific ideas.

LINQ - readability

For loops - speed

private static int FirstDuplicate2(int[] a)
{
    for (int i = 0; i < a.Length - 1; i++)
    {
        for (int j = i + 1; j < a.Length; j++)
        {
            if (a[i] == a[j])
            {
                return a[i];
            }                   
        }
    }
    return -1;
}

HashSet<> - middle-ground

It's often times easy to overthink a simple problem, such is your case, you've over-complicated a simple task. As you can see my for loop is a lot shorter, simpler, faster and easier to understand. You should think through the problem, before writing code. Some people prefer to write first and than improve afterwards, this also works, but it's always good to think a bit about it before-hand, especially when you're beginner you might often find yourself tunnel-visioned in a specific way of solving the problem once you're done with it, you could avoid that by thinking before-hand as you wont be so attached to specific ideas.

LINQ

HashSet<>

It's often times easy to overthink a simple problem, such is your case, you've over-complicated a simple task. You should think through the problem, before writing code. Some people prefer to write first and than improve afterwards, this also works, but it's always good to think a bit about it before-hand, especially when you're beginner you might often find yourself tunnel-visioned in a specific way of solving the problem once you're done with it, you could avoid that by thinking before-hand as you wont be so attached to specific ideas.

Source Link
Denis
  • 8.6k
  • 5
  • 33
  • 76

Performance is not the only important thing about a piece of code. Depending on the nature of the problem you might also need to consider - readability, maintainability and flexibility. I will present you couple of solutions focusing on those different aspects.

LINQ - readability

You could use LINQ and write it like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    foreach (var item in source)
    {
        if (source.Count(t => t == item) > 1)
        {
            return item;
        }
    }
    return -1;
}

If you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate(IEnumerable<int> source)
{
    return source.FirstOrDefault(item => source.Count(t => t == item) > 1);
}

LINQ is usually quite readable and most C# programmers are familiar with it's syntax so it's a lot easier to just give it a glance and know what's up.

For loops - speed

private static int FirstDuplicate2(int[] a)
{
    for (int i = 0; i < a.Length - 1; i++)
    {
        for (int j = i + 1; j < a.Length; j++)
        {
            if (a[i] == a[j])
            {
                return a[i];
            }                   
        }
    }
    return -1;
}

HashSet<> - middle-ground

The HashSet<T> is very handy when working with unique numbers as it can contain only distinct numbers. It's bool Add(T item) method will allow us to quickly check if a value was already added:

private static int FirstDuplicate(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    foreach (var item in source)
    {
        if (!distinctItems.Add(item))
        {
            return item;
        }
    }
    return -1;
}

Again, if you could've returned 0 instead of -1 in case no duplicates are found, it could've been written like this:

private static int FirstDuplicate<int>(IEnumerable<int> source)
{
    var distinctItems = new HashSet<int>();
    return source.FirstOrDefault(item => !distinctItems.Add(item));
}

Your code

It's often times easy to overthink a simple problem, such is your case, you've over-complicated a simple task. As you can see my for loop is a lot shorter, simpler, faster and easier to understand. You should think through the problem, before writing code. Some people prefer to write first and than improve afterwards, this also works, but it's always good to think a bit about it before-hand, especially when you're beginner you might often find yourself tunnel-visioned in a specific way of solving the problem once you're done with it, you could avoid that by thinking before-hand as you wont be so attached to specific ideas.

Missing for loop indentation is also reducing the readability of your function.