Coding Confession: how not to use LINQ



  • I had to update artworks in my MP3 collection, so I opened a project that I wrote a couple of months ago to help with this task. Unfortunately, I made a grave mistake of opening it in Visual Studio.

    Next mistake was to actually look at the code.

    private List<NameAndFullPath> getListMP3s(List<string> mp3Paths, bool withPictures)
            {
                return mp3Paths.Where(x => x.EndsWith("mp3"))
                    .Select(x =>
                    { 
                       var y = x.Split('\\'); 
                       var s = "";
                       for (var i = 0; i < y.Length - 1; i++) { s = s + y[i] + "\\"; }; 
                       return s; 
                    })
                    .Distinct()
                    .Where(x =>
                    {
                        var sampleFile = SearchDirectory(new DirectoryInfo(x)).ElementAtOrDefault(0);
                        if (sampleFile == null) return false;
                        var f = TagLib.File.Create(sampleFile);
                        if (f.Tag.Pictures.Length != 0) return withPictures;
                        return !withPictures;
                    })
                    .Where(x =>
                    {
                        if (withPictures) return true;
                        return !ignored.Contains(x);
                    })
                    .OrderBy(x => x.Split('\\')[x.Split('\\').Length - 2])
                    .Select(x => new NameAndFullPath 
                    { 
                        Name = x.Split('\\')[x.Split('\\').Length - 3] + " - " + x.Split('\\')[x.Split('\\').Length - 2], 
                        FullPath = x 
                    })
                    .ToList();
            }
    

    Yes, that is a single statement here. I don't know what kind of crack I was on when I wrote it, but it must've been good.


    Filed under: tempted to troll CodeReview SE with this


  • SockDev

    It almost looks like you're writing C# for node :laughing:
    @Maciejasjmj said:

    tempted to troll CodeReview SE with this

    Could you live with the number of exploded brains that would cause?



  • Jesus.

    You're writing entire functions in your where clauses.



  • It's really not that bad. Two Wheres in a row is weird.

    I think it's actually easier to maintain or refactor code like this because the kinds of operations you're trying to perform (Select, Where, OrderBy) are separated instead of having overlapping operations in the form of foreach and if statements.

    A little cleanup and it's pretty easy to understand:

    private List<NameAndFullPath> getListMP3s(List<string> paths, bool withPictures)
    {
        return paths.Where(x => GetExtension(x) == ".mp3")
            .Select(GetAlbumFolderName)
            .Distinct()
            .Where(x => HasSampleFile(x, withPictures) && (withPictures || !ignored.Contains(x)))
            .OrderBy(GetArtistFolderName)
            .Select(x => new NameAndFullPath
            { 
                Name = GetAristFolderName(x) + " - " + GetAlbumFolderName(x),
                FullPath = x
            })
            .ToList();
    }
    

    I'm guessing your folder are structured like:

    Music Folder
        Artist
            Album
                Song.mp3
    

    The really messy part of this code isn't the use of linq, it's the use of shit like x.Split('\\')[x.Split('\\').Length - 3] + " - " + x.Split('\\')[x.Split('\\').Length - 2]. You would have done just the same without linq.



  • @Maciejasjmj said:

    I don't know what kind of crack I was on when I wrote it,

    I recently said something nearly identical ("I must have been on drugs.") about something I wrote. Of course that was hyperbole; I can even vaguely remember why I did it that way, and it makes sense given the problem I was solving.

    But my first take on code that I wrote before, when I lacked tools or knowledge I now have--or to an earlier "standard"---often is one of bemusement: Why would I do that? Obviously, I didn't know better, then.



  • @Maciejasjmj said:

    I don't know what kind of crack I was on when I wrote it

    For interest sake, and because it does contain some LINQ, here is what code can look like when really written on drugs... (I was on meth when I wrote it.)

    I tended to write horribly verbose code back then, and for some bizarre reason I thought it was hilarious to catch AggregateExceptions, always with a variable called ax, just so I could type: ax.Handle... (It seemed funnier when I was high.)

    /// <summary>Populate the file and directory list from the zip file entries, keeping track of the current directory
    /// in a class-level property. We always only display the files and directories contained by the current directory.
    /// path == null indicates that the current directory is the root directory of the zip file.</summary>
    private void Populate(string path = null)
    {
        CurrentZipDirectory = path;
    
        // Use threadsafe collections to hold temporary lists of files and directories.
        var directoryBag = new ConcurrentBag<ListViewItem>();
        var fileBag = new ConcurrentBag<ListViewItem>();
    
        // This list will be filtered for the current directory.
        var zipFileEntries = Enumerable.Empty<ZipArchiveEntry>().ToList();
    
        try
        {
            using (var zipArchive = ZipFile.Open(Filename, ZipArchiveMode.Read))
            {
                try
                {
                    zipFileEntries = zipArchive.Entries.ToList();
                }
                catch (Exception ex)
                {
                    ex.Log();
                    MessageBox.Show(Browser.Instance, string.Format("The zip file appears to be invalid and could not be read.\n{0}", ex.Message), "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                    return;
                }
    
                // Discard everything that is not nested in/below the current directory
                if (path != null)
                    zipFileEntries.RemoveAll(entry => !(entry.FullName.StartsWith(path + Romy.Core.Constants.DelimiterChars[0]) || entry.FullName.StartsWith(path + Romy.Core.Constants.DelimiterChars[1])));
    
                /* Get the directories to display. Zip files sometimes contain entries for directories, but often
                 * do not, and when we parse the entries paths, there will be many recurrences of each directory.
                 * Thus use a dictionary to keep track of the directories we have already found. */
                var directories = new ConcurrentDictionary<string, string>();
    
                var directoryEntries = zipFileEntries.Where(e =>
                {
                    // Filter for subdirectories of the current directory.
                    var filenameInZip = e.FullName;
    
                    /* Remove current directory from string, so that the same
                     * string-manipulation logic applies to all directries. */
                    if (path != null)
                        filenameInZip = filenameInZip.Substring(path.Length + 1);
    
                    /* With the current directory removed, any string that contains one slash (forward or
                     * backslash; whatever this zip file uses) is a sub-directory of the current directory. */
                    if (filenameInZip.Count(c => c == '\\' || c == '/') > 0)
                    {
                        var directory = filenameInZip.Substring(0, filenameInZip.IndexOfAny(Romy.Core.Constants.DelimiterChars));
    
                        if (!directories.Values.Contains(directory))
                        {
                            directories[e.FullName] = directory;
                            return true;
                        }
                    }
                    return false;
                });
    
                try
                {
                    directoryEntries.AsParallel().ForAll(entry =>
                    {
                        var index = path == null ? 0 : path.Length + 1;
                        var relativePath = entry.FullName.Substring(0, entry.FullName.IndexOfAny(Romy.Core.Constants.DelimiterChars, index));
    
                        /* Use my SimpleFileInfo helper class to get the file or directory description and image
                         * index from the SystemImageList. (The SystemImagelist is used by the listview.) */
                        var simpleFileInfo = new SimpleFileInfo
                        {
                            Name = directories[entry.FullName],
                            FileSystemType = Romy.Core.FileSystemType.Directory,
                            //DateModified = entry.ModifyTime,  // This date in this entry refers to a file, not the directory. As we built the
                            Size = 0                            // dictionary of directories, we kind of hijacked a file entry to use for the directory.
                        };
    
                        directoryBag.Add(ShellListView.CreateListViewItem(new ZipEntryInfo
                        {
                            SimpleFileInfo = simpleFileInfo,
                            DisplayName = relativePath,
                            CompressedLength = entry.CompressedLength,
                            FullName = entry.FullName,
                            LastWriteTime = entry.LastWriteTime,
                            Length = entry.Length,
                            Name = entry.Name
                        }, simpleFileInfo));
                    });
                }
                catch (AggregateException ax)
                {
                    var handled = false;
    
                    ax.Handle(ex => handled = ex is ArgumentOutOfRangeException);
    
                    if (!handled)
                        throw;
                }
    
                /* Get the files. The logic is almost identical to the directories. */
                var fileEntries = zipFileEntries.Where(e =>
                {
                    return path != null ? e.FullName.IndexOfAny(Romy.Core.Constants.DelimiterChars, path.Length + 1) == -1 : e.FullName.IndexOfAny(Romy.Core.Constants.DelimiterChars) == -1;
                });
    
                try
                {
                    fileEntries.AsParallel().ForAll(entry =>
                    {
                        var relativePath = entry.FullName;
    
                        if (path != null)
                            relativePath = relativePath.IndexOf(Path.DirectorySeparatorChar) != -1 ? relativePath.Replace(path + Path.AltDirectorySeparatorChar, string.Empty) : relativePath.Replace(path + '/', string.Empty);
    
                        if (!string.IsNullOrEmpty(relativePath) && !(relativePath.EndsWith(Path.DirectorySeparatorChar.ToString()) && !(relativePath.EndsWith(Path.AltDirectorySeparatorChar.ToString()) && entry.Length == 0)))
                        {
                            var simpleFileInfo = new SimpleFileInfo
                            {
                                Name = relativePath,
                                FileSystemType = FileSystemType.File,
                                DateModified = entry.LastWriteTime.LocalDateTime,
                                Size = entry.Length
                            };
    
                            fileBag.Add(ShellListView.CreateListViewItem(new ZipEntryInfo
                            {
                                SimpleFileInfo = simpleFileInfo,
                                DisplayName = relativePath,
                                CompressedLength = entry.CompressedLength,
                                FullName = entry.FullName,
                                LastWriteTime = entry.LastWriteTime,
                                Length = entry.Length,
                                Name = entry.Name
                            }, simpleFileInfo));
                        }
                    });
                }
                catch (AggregateException ax)
                {
                    var handled = false;
    
                    ax.Handle(ex => handled = ex is ArgumentOutOfRangeException);
    
                    if (!handled)
                        throw;
                }
    
                // Copy our ConcurrentBag lists to arrays, and sort them in parallel.
                var directoryItemArray = directoryBag.ParallelSort(Comparer<ListViewItem>.Create((x, y) =>
                {
                    return string.Compare(((ZipEntryInfo)x.Tag).DisplayName, ((ZipEntryInfo)y.Tag).DisplayName, StringComparison.InvariantCultureIgnoreCase);
                })).ToArray();
    
                var fileItemArray = fileBag.ParallelSort(Comparer<ListViewItem>.Create((x, y) =>
                {
                    string xString = ((ZipEntryInfo)x.Tag).DisplayName;
                    string xExtension = Path.GetExtension(xString);
                    xString = Path.GetFileNameWithoutExtension(xString);
    
                    string yString = ((ZipEntryInfo)y.Tag).DisplayName;
                    string yExtension = Path.GetExtension(yString);
                    yString = Path.GetFileNameWithoutExtension(yString);
    
                    return (xExtension + xString).CompareIgnoreCultureAndCase(yExtension + yString);
                })).ToArray();
    
                listView.BeginUpdate();
                try
                {
                    listView.Items.Clear();
    
                    listView.Items.AddRange(directoryItemArray);
                    listView.Items.AddRange(fileItemArray);
    
                    if (fileItemArray.Length == 0)
                    {
                        sizeHeader.Text = string.Empty;
                        modifiedHeader.Text = string.Empty;
                    }
                    else
                    {
                        sizeHeader.Text = "Size";
                        modifiedHeader.Text = "Modified Date";
                    }
    
                    if (listView.Items.Count > 0)
                        listView.AutoResizeColumns(ColumnHeaderAutoResizeStyle.ColumnContent);
                }
                finally { listView.EndUpdate(); }
            }
    
            validZipFile = true;
        }
        catch (InvalidDataException ex)
        {
            ex.Log();
            MessageBox.Show(Browser.Instance, string.Format("The zip file appears to be invalid and could not be read.\n{0}", ex.Message), "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
            return;
        }
    
        UpdateStatusStripInfo();
        SelectLastDirectory();
    
        // The vertical scrollbar is not being painted unless we mouse-over it or force a repaint.
        // Thanks to writing my own crappy control instead of downloading a decent one.
        listView.Invalidate(true);
        listView.Update();
    
        InitializeControls();
    }


  • @CoyneTheDup said:

    I recently said something nearly identical ("I must have been on drugs.") about something I wrote

    Do you ever look back and think "I wish I'd been on drugs when I wrote this so there would be some excuse?"

    I found this the other day in legacy code I wrote in 2010. Drugs wouldn't have been enough of an excuse

    [code] if (ISSET($_POST['campus']) AND !ISSET($_GET['lim']))
    {
    $sql .= " WHERE product_campus = '$_POST[campus]' ORDER BY product_category, LTRIM(product_name) ASC";
    }
    else if(ISSET($_POST['campus']) AND ISSET($_GET['lim']))
    {
    $sql .= " WHERE product_campus = '$_POST[campus]' AND LTRIM(product_name) LIKE '".$_GET['lim']."%'";
    }
    else if(ISSET($_GET['campus']) AND ISSET($_GET['lim']))
    {
    $sql .= " WHERE product_campus = '$_GET[campus]' AND LTRIM(product_name) LIKE '".$_GET['lim']."%'";
    }
    else if (!ISSET($_POST['campus']) AND ISSET($_GET['lim']))
    {
    $sql .= " WHERE LTRIM(product_name) LIKE '".$_GET['lim']."%' ORDER BY LTRIM(product_name) ASC";
    }
    else
    {
    $sql .= " ORDER BY product_campus, product_category, LTRIM(product_name) ASC";
    }[/code]



  • We've all been there. You're like "This is soo neat! LINQ is AWESOME! All my code is LINQ from now on!". And then something throws and you realize that shit is IMPOSSIBLE to debug.

    At least it was back in yesteryear of 2013. There were noises MS was adding LINQ breakpoints in C# 5 CLR Core 6 or whatever rebranding they are into now.

    Anyway, WTF now, less so if they add LINQ debugging as they promised.



  • @cartman82 said:

    And then something throws and you realize that shit is IMPOSSIBLE to debug.

    That isn't really ever a problem; you've been able to break in linq for a while now (f9 while the cursor is inside the lambda).

    Or you can make a variable for each step rather than chaining.

    Or you can use methodgroups instead of lambdas.

    Why don't you know these things?



  • LINQ's ok to debug, what bothers me is it takes what would have been design-time type errors (when coding the old-fashioned way) and makes them into run-time type errors.

    I still use it pretty often.


  • Impossible Mission Players - A

    Weird, that PHP looks strangely similar to some code I developed about six months back.
    Granted, its purpose is to allow POSTed parameters to not exist and still succeed (After all, who in their right minds would truly need a coordinate pair for a Map point?!).
    It's rather fun when I get entries in the middle of the ocean of an unknown category with no name and no marker icon.



  • @Tsaukpaetra said:

    It's rather fun when I get entries in the middle of the ocean of an unknown category with no name and no marker icon.

    Maybe you found R'lyeh?


  • area_deu

    As everyone knows, there's an island called "Titiwu" at 0°N 0°E



  • You're scaring me



  • .split('.').last would be more appropriate?


  • Impossible Mission Players - A

    Only scaring? Then we have failed!



  • @thegoryone said:

    Do you ever look back and think "I wish I'd been on drugs when I wrote this so there would be some excuse?"

    Ummm...yeah, I think I could go with that. But the code needs to be older than a year.


  • Discourse touched me in a no-no place

    @aliceif said:

    As everyone knows, there's an island called "Titiwu" at 0°N 0°E

    They've got lots of members of every species there, at least according to the main database of species occurrences…


  • Discourse touched me in a no-no place

    @aliceif said:

    As everyone knows, there's an island called "Titiwu" at 0°N 0°E

    Some of our systems can be found there occasionally.



  • I don't know that you need to use LINQ functions for selecting the path parts. LINQ doesn't have a FromLast(int) that gives you like the nth last item or anything.

    But this

    x.Split('\\')[x.Split('\\').Length - 3] + " - " + x.Split('\\')[x.Split('\\').Length - 2]
    

    could be

    String GetNthFromLast(int nth, String path)
    {
        var parts = path.Split('\\');
        return parts.Length < nth + 1 ? "" : parts[parts.Length - nth - 1];
    }
    
    ...
    
    GetNthFromLast(2, x) + " - " + GetNthFromLast(1, x)
    

    It's easier to tell what that's doing.

    // made up syntax because c sharp sucks
    GetArtistFolderName = GetNthFromLast 2
    GetAlbumFolderName = GetNthFromLast 1
    

  • SockDev

    @Bort said:

    I don't know that you need to use LINQ functions for selecting the path parts. LINQ doesn't have a FromLast(int) that gives you like the nth last item or anything.

    No, but you can just Reverse().Skip(n).Take(1) instead.

    Obviously, if you know what type of collection you're using, you may be able to find a more efficient way. But if you just have an IEnumerable, then Reverse().Skip(n).Take(1) is all you have.


  • SockDev

    @RaceProUK said:

    Reverse().Skip(n).Take(1)

    Generally speaking I'd prefer Reverse().Skip(n).FirstOrDefault() myself. ;-)


  • SockDev

    To be fair, the Skip(n) is more likely to throw :stuck_out_tongue:


  • SockDev

    true, true.

    but then you should have some sort of protection against that already in place earlier in the code. ;-)



  • @RaceProUK said:

    more likely

    I don't like guessing.

    from System.Linq.Enumerable in System.Core.dll:

    /// <summary>Bypasses a specified number of elements in a sequence and then returns the remaining elements.</summary>
    /// <returns>An <see cref="T:System.Collections.Generic.IEnumerable`1" /> that contains the elements that occur after the specified index in the input sequence.</returns>
    /// <param name="source">An <see cref="T:System.Collections.Generic.IEnumerable`1" /> to return elements from.</param>
    /// <param name="count">The number of elements to skip before returning the remaining elements.</param>
    /// <typeparam name="TSource">The type of the elements of <paramref name="source" />.</typeparam>
    /// <exception cref="T:System.ArgumentNullException">
    ///   <paramref name="source" /> is null.</exception>
    [__DynamicallyInvokable]
    public static IEnumerable<TSource> Skip<TSource>(this IEnumerable<TSource> source, int count)
    {
    	if (source == null)
    	{
    		throw Error.ArgumentNull("source");
    	}
    	return Enumerable.SkipIterator<TSource>(source, count);
    }
    
    private static IEnumerable<TSource> SkipIterator<TSource>(IEnumerable<TSource> source, int count)
    {
    	using (IEnumerator<TSource> enumerator = source.GetEnumerator())
    	{
    		while (count > 0 && enumerator.MoveNext())
    		{
    			count--;
    		}
    		if (count <= 0)
    		{
    			while (enumerator.MoveNext())
    			{
    				yield return enumerator.Current;
    			}
    		}
    	}
    	yield break;
    }
    

    @RaceProUK said:

    Reverse().Skip(n).Take(1)

    That's about as bad as what he has now.


  • SockDev

    @Bort said:

    from System.Linq.Enumerable in System.Core.dll:

    huh... so skip won't throw if you try to skip 1000 items in a 10 item IEnumerable.

    good to know!


  • SockDev

    @accalia said:

    huh... so skip won't throw if you try to skip 1000 items in a 10 item IEnumerable.

    In which case, Reverse().Skip(n).FirstOrDefault() is preferable ;)


  • BINNED

    I don't know enough about .NET or LINQ to be sure, but it sounds to me like you're all pretty much discussing on how to navigate an array.

    This shit used to be easy!


    Filed under: [ ]


  • SockDev

    @Onyx said:

    I don't know enough about .NET or LINQ to be sure, but it sounds to me like you're all pretty much discussing on how to navigate an array collection in a manner independent of how the collection is implemented.

    Which is kinda the whole point of LINQ; list, array, queue, stack, it doesn't matter, so long as it's IEnumerable ;)



  • @Onyx said:

    I don't know enough about .NET or LINQ to be sure, but it sounds to me like you're all pretty much discussing on how to navigate an array.

    There are about a gazillion off-by-one segfaults that would like to disagree with you.

    LINQ is pretty much functional-programming-light (for collections)



  • @blakeyrat said:

    LINQ's ok to debug, what bothers me is it takes what would have been design-time type errors (when coding the old-fashioned way) and makes them into run-time type errors.

    What do you mean by that? I can't think of anything that would be run-time errors with LINQ but compile-time errors without it.
    If you're not using generics you might have runtime cast problems, but that's the same with non-LINQ code.


  • ♿

    @dtech said:

    What do you mean by that? I can't think of anything that would be run-time errors with LINQ but compile-time errors without it.

    Probably this sort of thing:

    http://what.thedailywtf.com/t/quick-linq-question/47849/3?u=boomzilla



  • @boomzilla said:

    Probably this sort of thing:

    Interesting case, although it's more of a C# type system weakness than a LINQ problem per se. LINQ/functional style coding is probably more likely to reveal such weaknesses though, that's why FP languages bring the whole shebang.



  • LINQ's hardly functional (yes, you pass lambdas, but that's more out of necessity - you aren't supposed to stuff complex functions in there, as evidenced above).

    It's more declarative, like SQL. Hell, it basically is SQL, adapted to C# type system and syntax.



  • @Maciejasjmj said:

    (yes, you pass lambdas, but that's more out of necessity - you aren't supposed to stuff complex functions in there, as evidenced above).

    Or you pass in functions. Which is fairly functional, imo.


  • BINNED

    This post is deleted!

  • SockDev

    @Magus said:

    Or you pass in functions. Which is fairly functional, imo.

    True, but in C#, functions aren't a first-class citizen; you have to use one of the many Action<> or Func<> classes to represent them. And even then, they're limited. And that's not including some APIs which want Predicate<> or Delegate<>.



  • @RaceProUK said:

    True, but in C#, functions aren't a first-class citizen; you have to use one of the many Action<> or Func<> classes to represent them. And even then, they're limited. And that's not including some APIs which want Predicate<> or Delegate<>.

    What does that have to do with anything? I'm saying you can:

      var b = things.Select(SomeFunction);
    }
    
    public string SomeFunction(Thing thing)
    {
      ...
    

    What delegate types .NET provides by default are irrelevant to that.



  • But they are a first-class citizen, just check the IL. Action/Func/Predicate/Delgate and the delegate definition system is there to enforce strong typing, to ensure the function you're calling can use the delegate/callback you pass in.


  • SockDev

    @TwelveBaud said:

    But they are a first-class citizen, just check the IL

    They may be in IL, but who writes straight IL? Almost no-one. Most write in C#, where they're not first-class.

    Basically, if I can't do
    var f = () => return true;
    without explicitly referring to Func<bool>, then functions aren't first-class citizens.



  • @Magus said:

    Or you pass in functions. Which is fairly functional, imo.

    Does SELECT * FROM SomeTable WHERE Func(SomeTable) make SQL functional?

    You can make LINQ somewhat functional, but to me it's more of a side effect of C# having no facilities to specify a property in a strongly-typed way than a conscious decision.



  • Look, if passing functions as arguments doesn't classify as 'functional' to you, I suggest you go read something.


  • SockDev

    In C#, you don't pass functions, you pass objects that represent functions


  • BINNED

    @Magus said:

    Look, if passing functions as arguments doesn't classify as 'functional' to you, I suggest you go read something.

    So, JavaScript is functional? Even though

    In functional code, the output value of a function depends only on the arguments that are input to the function, so calling a function f twice with the same value for an argument x will produce the same result f(x) each time.

    Happens very rarely in the most commonly used case where you pass in functions: callbacks to functions that manipulate the DOM. Because most of the time the result will depend on the current state of the DOM.



  • * TwelveBaud sighs...

    delegate bool BoolFunc(); BoolFunc func = () => { return true; };

    If that's still not good enough for you, PRs accepted.


  • SockDev

    And BoolFunc is an object, not a function, which was my original point :stuck_out_tongue:



  • Javascript functions are objects too. So there. *pffbbfbfbfbft*

    <!-- ;) -->


  • You've passed the level of pedantry where what you posted means anything. If I pass a methodgroup, and the end result is the same as if I'd passed the function directly somehow, I don't really care. This constitutes no difference in the way I use it.

    @Onyx said:

    >In functional code, the output value of a function depends only on the arguments that are input to the function, so calling a function f twice with the same value for an argument x will produce the same result f(x) each time.

    This looks like the kind of function I pass to linq, because I'm not an idiot.


  • SockDev

    JS only has one function type though: function :stuck_out_tongue:


  • BINNED

    @Magus said:

    This looks like the kind of function I pass to linq, because I'm not an idiot.

    Fine, but then the statement:

    @Magus said:

    Look, if passing functions as arguments doesn't classify as 'functional' to you, I suggest you go read something.

    is not the only requisite for a language to be functional. It should at least encourage, if not enforce this. JS won't, but passing functions along is as common as passing numbers or objects.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.