Trying out this LINQ quasi-SQL syntax



  • I started with this code:

    foreach (i in Enumerable.Range(x, ship.size))
    {
        if (blocked[i, y])
        {
            return false;
        }
    }
    return true
    

    Because I like functional style when it comes to loops, I've rewritten it as this:

    return !Enumerable.Range(x, ship.size).Select(i => blocked[i, y]).Aggregate((a, b) => a || b);
    

    I thought I might try out this special keyword-mashup syntax C# offers. I came up with this:

    return !(from i in Enumerable.Range(x, ship.size) select blocked[i, y]).Aggregate((a, b) => a || b);
    

    But I'm not quite satisfied, because there's still this ugly call to Aggregate. I went googling how to replace it with pretty magic keywords, only to learn that it's impossible. But I thought I'd make use of where keyword. So I tried this instead:

    return !(from i in Enumerable.Range(x, ship.size) where blocked[i, y] select i).Any();
    

    But it still itches my eye because of the totally unnecessary select statement.

    Is there a way to "fix" this "problem"? I just want to return false if any element is true. I don't really care how, as long as it's nice and short.



  • Maybe I'm an idiot, but why can't you do:

    return !(Enumerable.Range(x, ship.size).Any(i => i.blocked[i, y]))
    

    I don't know where y comes from though.

    I also don't understand the need for the aggregate or the select, frankly. So I might be way off-base here.


  • area_deu

    +1 for Any()



  • @blakeyrat said:

    Maybe I'm an idiot, but why can't you do:

    Because I learned LINQ five minutes ago, and my total accumulated experience with C# is like nine hours throughout years, six of which were just messing around with Windows Forms editor.

    Anyway, thanks!

    BTW - redundant parentheses are redundant.



  • My personal style guys says that if you're negating a thing with a period somewhere in it, you use a paren for clarity.

    Ok:

    !blob;
    !blob(monsterExample);
    

    Not ok:

    !blob.MonsterExample(foo);
    

    Ok:

    !(blob.MonsterExample(foo));
    

    Anyway, it's your code, do what you want. No need to chide me.



  • @Gaska said:

    keyword-mashup syntax

    It's confusing for no good reason. Drop it and just chain functions.

    And if you did that, I think you'd see much more easily that there's no need for a Select call (which takes each element and transforms it) if you're just going to call Any() (which counts the elements) right after. Basic LINQ is much easier to digest when you think in terms of applying transforms on collections, not running SQL-style queries.



  • @Gaska said:

    Because I like functional style when it comes to loops, I've rewritten it as this:

    This whole topic reads to me like an excellent argument why one should stay away from functional style when it comes to loops.

    I wouldn't normally have said that in here, but since your actual question has already been solved, I couldn't resist putting in my 2¢.


  • Discourse touched me in a no-no place

    @boomzilla said:

    This whole topic reads to me like an excellent argument why one should stay away from functional style when it comes to loops.

    Ideally, you avoid having to explicitly loop over things in the first place with functional style, instead just saying that you apply an operation to each member of a collection or filter a collection by some predicate.



  • I know what functional style is. I'm really just saying this is the Hitler of examples or something. I guess it's good for @Gaska learning LINQ or whatever, but it's shit for readability compared to what he started with.



  • I think you just don't like functional programming.



  • To be frank, both styles look like shit here, because you're trying to iterate over blocked, but for whatever reason, you're looping on enumerable.range instead. The way to make this make sense using functional style would be

    return blocked.Take(ship.size).Skip(x).All(isBlocked => !isBlocked[y]);
    

    Although presumably you can't iterate over the rows of blocked like that, hence why you did it this upside-down way in the first place. In that case, a regular for(var i=x; i<ship.size; i++) would still have been clearer than that ridiculous foreach.


    Filed under: carrying on personal vendettas in coding help



  • @Buddy said:

    To be frank, both styles look like shit here, because you're trying to iterate over blocked, but for whatever reason, you're looping on enumerable.range instead.

    That's because I want only the elements of blocked at specific indexes.

    @Buddy said:

    ```
    return blocked.Take(ship.size).Skip(x).All(isBlocked => !isBlocked[y]);

    1. You messed up the order of Take and Skip.
    2. This approach requires iterating from the beginning of row every time, just to ignore first x elements. While it makes sense for streaming (or arbitrary) iterators, it's stupid to do over an array.
    3. You obviously omitted the part where you tell if you're iterating over the rows or columns of the 2-dimensional array. It's understandable, since C# has no such functionality (it doesn't even implement LINQ interface for multi-arrays to start with!) - nevertheless, I bet that if it had, it would make your cute line less understandable, or at least less pretty.
    4. If it was me, I would use LINQ to select both row and column, not just one of them and then get the other via array syntax. Be consistent.
    5. Unnecessarily long variable name in lambda expression is unnecessarily long.
    
    @Buddy <a href="/t/via-quote/50848/11">said</a>:<blockquote>Although presumably you can't iterate over the rows of blocked like that, hence why you did it this upside-down way in the first place. In that case, a regular for(var i=x; i&lt;ship.size; i++) would still have been clearer than that ridiculous foreach.</blockquote>
    What's ridiculous in that `foreach` that isn't ridiculous in this `for`, except it's less portable if you ever wanted to compile my .cs file with GCC? And except this stupid "Enumerable." in front - but if you know C# well, you should have already learned to not read this word.


  • I like functional style, but I agree with @boomzilla. Too much terseness can hurt readability.



  • @Gaska said:

    1. You messed up the order of Take and Skip.

    Oh I see, range takes a start and count, not a start and end. Well guess what: I don't give a shit. My way makes it obvious exactly how many elements are being taken from where, and if that makes spotting bugs in my code easier that's exactly the thing point.

    1. This approach requires iterating from the beginning of row every time, just to ignore first x elements. While it makes sense for streaming (or arbitrary) iterators, it's stupid to do over an array.

    No it doesn't.

    1. You obviously omitted the part where you tell if you're iterating over the rows or columns of the 2-dimensional array. It's understandable, since C# has no such functionality (it doesn't even implement LINQ interface for multi-arrays to start with!) - nevertheless, I bet that if it had, it would make your cute line less understandable, or at least less pretty.

    Because if you can't iterate over your object, you shouldn't be using linq on it, like I said in the first place.

    1. If it was me, I would use LINQ to select both row and column, not just one of them and then get the other via array syntax. Be consistent.

    You aren't doing a consistent thing. You're iterating the rows, and pulling out an arbitrary column. Using the correct syntax for pulling an arbitrary column out of an array is the clearest way to express that.

    1. Unnecessarily long variable name in lambda expression is unnecessarily long.

    It's nice there, because that way you can read the line from left to right and get a good overview of what's going on. If I knew more about what your code does i could have picked an even better name for it, and for ‘y’, so that we wouldn't be sitting here wondering wtf we're even looking at in the first place.

    @Gaska said:

    What's ridiculous in that foreach

    What's ridiculous is that you're creating an object to produce indexes for you for absolutely no reason, when there's already an idiomatic way to do that.

    Again: if the object you are iterating over isn't the the target of the iteration function, you really need to rethink your approach.



  • @Buddy said:

    Oh I see, range takes a start and count, not a start and end. Well guess what: I don't give a shit.

    You don't give a shit that your code is no-op whenever ship size is less than its X coordinate, and if it's not, it does the wrong thing anyway except for X=0? OK.

    @Buddy said:

    My way makes it obvious exactly how many elements are being taken from where

    Mine too. You can argue that C#'s Range should take start and end, not start and count, but that's beside the point. Both our approaches are pretty equivalent here, except I'm using indexes and you're using fields.

    @Buddy said:

    No it doesn't.

    I don't know the internals of LINQ and .NET JIT, so I can't say for sure, but I'm not exactly convinced the Skip() would be optimized out.

    @Buddy said:

    Because if you can't iterate over your object, you shouldn't be using linq on it, like I said in the first place.

    Depends on what you mean by iterate. And what you refer to by "your object".

    @Buddy said:

    You aren't doing a consistent thing. You're iterating the rows, and pulling out an arbitrary column.

    I'm iterating over range of integers and accessing cells in array by providing both indices. That's consistent to me.

    @Buddy said:

    Using the correct syntax for pulling an arbitrary column out of an array is the clearest way to express that.

    Alas, C# doesn't have that.

    @Buddy said:

    It's nice there, because that way you can read the line from left to right and get a good overview of what's going on.

    Thanks to the "blocked" at the very beginning, not thanks to the duplicated "isBlocked" at the end. If anything, your line makes me think that you're checking if blocked is blocked.

    @Buddy said:

    If I knew more about what your code does i could have picked an even better name for it

    Like i. Because the scopes of lambdas are so small that any non-single-letter identifier is just code bloat. And you don't need to know context to know that.

    @Buddy said:

    and for ‘y’, so that we wouldn't be sitting here wondering wtf we're even looking at in the first place.

    I'll provide more context at the end of this post.

    @Buddy said:

    What's ridiculous is that you're creating an object to produce indexes for you for absolutely no reason, when there's already an idiomatic way to do that.

    2007 called. They want to let you know of all the new idioms for iterating over stuff introduced in C# 3.0.

    @Buddy said:

    Again: if the object you are iterating over isn't the the target of the iteration function, you really need to rethink your approach.

    Somehow, this doesn't apply when the iteration is done with old-style for loop, apparently...


    OK, some context about what I'm doing. I'm making a game of battleships. The code in OP is from ship placement function - I need to know if the squares the ship is being placed at are already occupied. I have a 2D array of bools that keeps track of fields that are already occupied (called blocked). Ship is defined with coordinates of top left corner (x and y in code), direction (horizontal or vertical), and length (size variable; width of ship is always one field, so I don't need to store it). When ship is horizontal, I check all occupied y for given x, and if it's vertical, it's the other way around.



  • Oh, and thanks for letting me know of .All() function! Will make my code teeny tiny bit cleaner (no negating of whole expression, just the result of lambda).



  • @Gaska said:

    I think you just don't like functional programming.

    Not when it's worse than the equivalent in an imperative idiom.

    Figuring stuff out like you did is fun to write. Just not as fun to read and deal with later. That partly depends on the subtleties involved with the actual problem being solved.



  • @boomzilla said:

    Figuring stuff out like you did is fun to write. Just not as fun to read and deal with later.

    Shitty code is shitty regardless of paradigm. Functional style at least has the property of not randomly changing random variables everywhere...



  • @Gaska said:

    Shitty code is shitty regardless of paradigm

    Exactly my point.



  • I had a feeling that you're suggesting that functional programming is more shitty on average, or has higher chance of turning out shitty, compared to imperative.



  • I think it often tends to get extremely terse, relatively speaking. Stuff like LINQ also seems to be very dense with stuff going on. Here, I'll compress your original to make it (mostly) a one liner:

    foreach (i in Enumerable.Range(x, ship.size)){  if (blocked[i, y])  {  return false;  } }
    return true;
    

    Now, your preferred solution:

    return !(Enumerable.Range(x, ship.size).Any(i => i.blocked[i, y]))
    

    That's actually a lot better than what you originally posted. To me, the original loop is still clearer, because the intent of it really jumps out just by reading it. To understand the LINQ version, it seems like there's more jumping around, looking at the different bits to put it all together.

    I could easily imagine circumstances where the styles were reversed as far as that goes.



  • @boomzilla said:

    Now, your preferred solution:

    Update:

    return Enumerable.Range(x, ship.size).All(i => !blocked[i, y]);
    


  • @boomzilla said:

    To me, the original loop is still clearer, because the intent of it really jumps out just by reading it.

    Well, I despise the loop { if(cond) return false; } return true; pattern. It feels so much like a missed opportunity in this age. Kinda like making tiny class with single one-liner method instead of lambda (eg. as comparator in sort function).

    Also, I can imagine someone taking the imperative version and merging the "check if blocked" loop with "mark as blocked" loop that's several lines later in the code.



  • @Gaska said:

    Well, I despise the loop { if(cond) return false; } return true; pattern. It feels so much like a missed opportunity in this age.

    I kind of agree with you, but in a...it's the worst thing, except for all the alternatives...sort of way.

    @Gaska said:

    Kinda like making tiny class with single one-liner method instead of lambda (eg. as comparator in sort function).

    I go back and forth on this sort of thing. On the one hand, yeah, it seems a little wasteful, but on the other hand, it also seems clearer to give something an actual name and stuff, instead of some random anonymous thing sitting around in your code. I am not at all consistent in this respect.



  • @Zecc said:

    I like functional style, but I agree with @boomzilla. Too much terseness can hurt readability.

    Tell that those programmers that code as if they were charged a buck per key stroke.

    Anyway, I'm trying to stick to a style that enables me to maintain my own code six months later after having worked on half a dozen other projects with different languages and different style conventions. I wish to be able to kick the ass of my younger self often enough without that.



  • @boomzilla said:

    I go back and forth on this sort of thing. On the one hand, yeah, it seems a little wasteful, but on the other hand, it also seems clearer to give something an actual name and stuff, instead of some random anonymous thing sitting around in your code.

    If lambda is instantly obvious in what it does, it doesn't need to be named. And if it's not obvious, it shouldn't be lambda.



  • @Gaska said:

    is instantly obvious

    Ah, the scariest words that a math teacher can utter..."It is therefore obvious that..." Obviousness is like beauty. It's entirely in the eye of the beholder.

    I'm not sure I necessarily agree with your criterion for legitimate lambda-hood. But like I said, I'm not at all consistent about this, so I guess that's obvious,. :stuck_out_tongue:



  • @boomzilla said:

    Ah, the scariest words that a math teacher can utter..."It is therefore obvious that..." Obviousness is like beauty. It's entirely in the eye of the beholder.

    As mathematicians in Germany say, "Wie man leicht sieht" ("As can easily be seen") is defined to mean the opposite.


  • area_deu



  • To me, in the LINQ version, it's really not clear what you're iterating over. What does Range(x,ship.size) represent? Clearer might be:

    return Enumerable.Range(0, ship.size).All(i => !blocked[x+i, y]);
    

    Although this version is slightly more code, it is now clear what we are iterating over - the length of the ship.



  • @PleegWat said:

    What does Range(x,ship.size) represent?

    RTFM



  • You just need more extension methods:

    public static IEnumerable<A> Column<A>(this A[][] matrix, int col)
    {
    	return matrix.Select(row => row[col]);
    }
    
    public static IEnumerable<A> Slice<A>(this IEnumerable<A> e, int from, int length)
    {
    	return e.Skip(from).Take(length);
    }
    
    public static bool AnyTrue(this IEnumerable<bool> e)
    {
    	return e.Any(x => x);
    }
    

    Then you can write:

    
    blocked.Column(y).Slice(x, ship.size).AnyTrue()
    

    Extension methods!


    The issue isn't LINQ or functional vs imperative, it's about expressing code in a way that relates to the problem. Are any (cells?) blocked in column y starting from x, running as long as the size of ship?


  • Discourse touched me in a no-no place

    @boomzilla said:

    Ah, the scariest words that a math teacher can utter..."It is therefore obvious that..."

    What's fun is when they work hard for an extended period of time to prove that something actually is obvious. :smiley:



  • @Bort said:

    You just need more extension methods

    Thanks but no thanks. These methods are too short, the usage count too small, and the original code too simple to be worth writing out.

    Not to mention you're using the wrong array type.



  • @dkf said:

    What's fun is when they work hard for an extended period of time to prove that something actually is obvious.

    They know that everything is obvious in hindsight, so they give the students enough hindsight to make it obvious. It can take up to three school hours, though.



  • @Gaska said:

    @PleegWat said:
    What does Range(x,ship.size) represent?

    RTFM

    That was a rhetorical question. It does not represent anything very clear - it's a range of numbers that has some relation to the ship and some relation to the grid.

    On the other hand, Range(0, ship.size) represents the range of positions the ship is occupying (in any direction) which I then map onto the grid in a second step.
    In an alternative approach, you could write a ship.positions list which returns a list of (x,y) coordinates the ship occupies (again when moving in a certain direction).

    When working with something like LINQ, it is important to be able to know at all stages, in as simple terms as possible what your variables/values/streams represent.



  • @PleegWat said:

    It does not represent anything very clear - it's a range of numbers that has some relation to the ship and some relation to the grid.

    Domain knowledge helps. If you know that Range takes the first value and number of consecutive values, then you look at the code and know that the range encompasses ship.size numbers from x onwards. If you know that x is the coordinate of the ship on the grid, you can easily deduce that the range is coordinates of all individual cells on the grid occupied by the ship.

    @PleegWat said:

    On the other hand, Range(0, ship.size) represents the range of positions the ship is occupying (in any direction) which I then map onto the grid in a second step.

    One could argue that someone could find your mapping as bizzare as you find my range, for exactly the same reason. If someone knows what all the variables mean, both approaches are just as clean, and if they don't, both are confusing.



  • @Gaska said:

    Not to mention you're using the wrong array type.

    No, you're using the wrong array type!



  • I'm using multidimensional array. You're using array of arrays. Since I need multidimensional array, my type is more correct than yours. Also, while your code is short and nice in one dimension, there's no easy way to do the same in the other - and I need both.



  • @Gaska said:

    I'm using multidimensional array. You're using array of arrays.

    Seems the only sensible way is to use a dictionary of coordinate pairs. And to give each ship a list of the coordinate pairs it occupies. Or another representation those can be deducted from.

    On a second thought no - that would be too complicated for others.


  • Discourse touched me in a no-no place

    @PWolff said:

    Seems the only sensible way is to use a dictionary of coordinate pairs. And to give each ship a list of the coordinate pairs it occupies. Or another representation those can be deducted from.

    A multidimensional (i.e., 2D) array is actually a sane model of a Battleships game board.



  • @dkf said:

    sane model

    Exactly.


Log in to reply
 

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