posted
Blayne, do exactly what I describe. Create ten items. Check #2, #7, #9, and #10. Click your "Delete" button.
Posts: 37449 | Registered: May 1999
| IP: Logged |
Blayne Bradley
unregistered
posted
I did do what Tom showed, works just fine on my end.
IP: Logged |
posted
I'd expect that when you went to delete #7, you'd actually delete #8 (because the index is off by one at that point because of the previous delete of #2).
Note that deleting a single row will work. Also, because of your if/else, deleting x number of rows at the end will also work. What I don't expect to work is deleting a row in the middle, then another row in the middle.
Edit: No, deleting at the end won't work either. It'll start deleting rows from the beginning once the index gets high enough. Deleting ALL rows will work though, since you'll always delete the quantity of rows that you are asking to delete, just not the rights ones, but if you delete all rows than it doesn't matter that you aren't deleting the correct ones since your if/else fall back will cause the first or last row to always be deleted once the index goes wonky.
Again, think about it. You already know the index is getting out of sync with the rows, but the only time you deal with that is when the index exceeds the total number of rows. How do you make sure that the index matches the middle rows once you've started deleting some? Why would it be off by one (or more) only when it's on the last row?
posted
THere's a difference between didn't throw an error and works fine. If you did what Tom described, you'd end up with 3 4 5 6 7 9 10
Posts: 10177 | Registered: Apr 2001
| IP: Logged |
posted
Oops. I was having doubts and deleted my post. Sorry about that.
Posts: 3275 | Registered: May 2007
| IP: Logged |
Blayne Bradley
unregistered
posted
I just had to grandstand, I could have just left it alone but nooooooo my 6th sense just had to force me to double check and no of course I just had to stick to my guns I just couldn't stay quiet, fix my code, I just had to make an ass of myself before realizing I was slightly a smigden mistaken.
code:
Dim count = 0 For Each GridViewRow1 As GridViewRow In GridView1.Rows '---Look at each checkbox here CheckBox1 = CType(GridViewRow1.FindControl("chkSelector"), CheckBox)
If CheckBox1.Checked Then 'chkBox = True
'---Get the database id KeyId = GridViewRow1.RowIndex 'KeyId = CType(GridView1.DataKeys.Item(GridViewRow1.RowIndex).Value, Int32) '---Use the KeytId to delete from the database.
If KeyId = gt.Rows.Count Then gt.Rows(KeyId - 1).Delete() ElseIf KeyId > gt.Rows.Count Then gt.Rows(KeyId - KeyId).Delete() Else If count = 0 Then gt.Rows(KeyId).Delete() Else gt.Rows(KeyId - count).Delete() End If End If count = count + 1 End If
Next GridView1.DataSource = gt GridView1.DataBind()
posted
That's still not going to work. And like 4 people have told you exactly how to address this problem.
Posts: 10177 | Registered: Apr 2001
| IP: Logged |
Blayne Bradley
unregistered
posted
*blink* okay I am 101% positive that this does the job, as the rows merge it now anticipates it so that it will delete the proper rows regardless of how far they've moved sicne each time I delete something I am 1 row off, thus if I dlete 3 things I am 3 rows off, hence, I use a counter to keep track of how many times I dleted something and adjust what row to dlete accordingly.
I have taken 9 rows, dleted 2, 5, and 7 and I got the correct results only the rows I wanted to dlete were dleted, none were shunted.
Maybe the way those "four" people said would work, but I know that my way also works, code is code there is more then one way to crack a duck.
IP: Logged |
posted
Huge difference between compiling and working. Using your code will result in rows being deleted that shouldn't be.
Also (and man do you not deserve this and you won't appreciate it), your solution even assuming you tweak it so that it actually works is a very poor one for a common issue. You should learn the proper way to do this that people have been hitting you over the head with because, if you show someone a code sample with this sort of coding in it, you'll mark yourself as a very bad programmer.
Posts: 10177 | Registered: Apr 2001
| IP: Logged |
quote:*blink* okay I am 101% positive that this does the job, as the rows merge it now anticipates it so that it will delete the proper rows regardless of how far they've moved sicne each time I delete something I am 1 row off, thus if I dlete 3 things I am 3 rows off, hence, I use a counter to keep track of how many times I dleted something and adjust what row to dlete accordingly.
If I understand you, you intended to add a bit of code (probably after you realized from my example that we were right) that increments every time you delete a row, and then subtracts that from the gridviewrow index to retrieve the new index of the datarow.
(I note, by the way, that the code you've listed above does not actually do this. Your "if" statements here actually prevent this from working the way you would expect. You would be far better off just declaring x to be 0 when you enter the foreach, then incrementing x every pass and deleting x from every index.)
That should work, provided none of your deletes fail for some reason; I'd probably throw up a try...catch around the whole thing just to be sure. That said, it seems remarkably clumsy compared to just -- as everyone else here has suggested -- iterating through the collection from right to left.
Posts: 37449 | Registered: May 1999
| IP: Logged |
So while Blayne's code doesn't work, and indeed this method is clumsier than it needs to be, you could conceptually subtract one from the index value per item deleted.
Posts: 37449 | Registered: May 1999
| IP: Logged |
2) Rather than using a "for" or "foreach" loop, use a temporary variable as the position counter, only increment the counter when I'm *not* deleting, and only loop until the counter is greater than the size of the array (no, I'm not going to write code for this now...).
Posts: 3486 | Registered: Sep 2002
| IP: Logged |
Blayne Bradley
unregistered
posted
k, i revised the code once again for while it worked for 7, it didnt quite work for 10 as by this point it would go into the KeyId-KeyId if case and screw up my results BUT by expressing that if there's avalid rows past the current row and KeyId > gt.rows.count.
Thus now it goes where its supposed to go and now works even with Toms example.
IP: Logged |
posted
Yes, it's possible to keep track of how many deletes you've done and update a delta variable as you go along, but that's hard to read, hard to maintain, and unnecessarily complex.
Even if you do decide to go that route, all of those 'if's and 'else's are not necessary. The only 'if' you need is the "If CheckBox1.Checked Then" line. Anything more than that is an indication that you don't understand what exactly is happening.
For instance, why would you need to conditionally subtract count only when it's non-zero? If count is zero, subtracting it has no effect so there is no need for this:
code:
If count = 0 Then gt.Rows(KeyId).Delete() Else gt.Rows(KeyId - count).Delete() End If
and you can just have this:
code:
gt.Rows(KeyId - count).Delete()
Similarly, if you are properly incrementing the count variable, there is no need for the checks to see if index exceeds count.
Posts: 3275 | Registered: May 2007
| IP: Logged |
posted
Don't like one SQL statement myself either becuase I need to write more code to maintain the collection array and eventually convert that in to the actual SQL statement.
Pseudo-code... in C#, 'cause I don't have to think about the syntax as much.
code:
int iPos = 0 while ( iPos < gt.Rows.Count ) { if ( delete row iPos? ) delete row from DB using the unique key delete "gt.Rows[iPos]" from collection else iPos++ }
This accounts for "gt.Rows" shrinking during the delete.
Posts: 3486 | Registered: Sep 2002
| IP: Logged |
posted
The sad thing about all of Blayne's "fixes" is that, as he fumbles closer and closer to discovering the things we're already telling him, I'm being more and more convinced that at no time has he understood anything we've said to him, or even why his own workarounds are "working."
Posts: 37449 | Registered: May 1999
| IP: Logged |
posted
Actually, it just occurred to me why Blayne might be resisting our suggestions to iterate backwards; he might not know how, since you can't use foreach backwards. He'll need a for loop for that, and his teacher may not have shown him that.
Posts: 37449 | Registered: May 1999
| IP: Logged |
quote:Originally posted by TomDavidson: Actually, it just occurred to me why Blayne might be resisting our suggestions to iterate backwards; he might not know how, since you can't use foreach backwards. He'll need a for loop for that, and his teacher may not have shown him that.
If his teacher has been doing data binding, collection management and user interfaces before doing a "for loop", then I refuse to believe this is a "programming" course.
*EDIT*
MattP, don't you need a "Step -1"? I admit I haven't done that myself in VB for quite some time...
Posts: 3486 | Registered: Sep 2002
| IP: Logged |
quote:Originally posted by Blayne Bradley: I only know how to loop backwards in C.
For (e=0;e<10;e--)
Er... check that again. That code right there is going to loop for a lot longer than you probably want it to.
Edit: or, depending on the data type, I suppose it might be shorter than you intend. At any rate, it's likely not doing what you want it to do.
Posts: 2437 | Registered: Apr 2005
| IP: Logged |
posted
*still don't get why you are making the row index do double duty as a unique key for the item. Just (in this case, manually create one or) have the db autogenerate a key (in the non-trivial implementation you are working up to). Then when populating the datagrid, set the datagrid row's key to an actual unique key (and not a changing index value). Then just delete the row whose unique key is equal to the one that's selected. This is standard, whether in datagrids, listboxes, dropdowns, etc. That way, the ORDER you delete is completely irrelevant, and you can be sure that the only thing being deleted is an item that has a key that is truly unique and equal to the one you want. (Similar to dagonee's SQL statement)
Posts: 1346 | Registered: Jun 1999
| IP: Logged |
quote:Don't like one SQL statement myself either becuase I need to write more code to maintain the collection array and eventually convert that in to the actual SQL statement.
The code to make the SQL statement was a single function call for me.
The big difference was that I was doing stateless* ASP, so I didn't have to coordinate with a UI control at all - I didn't even have that option.
*It wasn't entirely stateless - I had a user context - but we couldn't save anything like a datacontrol to the session.
Posts: 26071 | Registered: Oct 2003
| IP: Logged |
Blayne Bradley
unregistered
posted
I know my C was incorrect but I'm too lazy to hit the edit button, ironically the post to explain why i didn't edit it took more calories.
IP: Logged |
Blayne Bradley
unregistered
posted
code:
Dim count = 0 For Each GridViewRow1 As GridViewRow In GridView1.Rows '---Look at each checkbox here CheckBox1 = CType(GridViewRow1.FindControl("chkSelector"), CheckBox)
If CheckBox1.Checked Then 'chkBox = True
'Dim ProductID As String = GridView1.DataKeys(GridViewRow1.RowIndex)("pr_code").ToString '---Get the database id KeyId = GridViewRow1.RowIndex 'KeyId = CType(GridView1.DataKeys.Item(GridViewRow1.RowIndex).Value, Int32) '---Use the KeytId to delete from the database.
If KeyId = gt.Rows.Count Then gt.Rows(KeyId - 1).Delete() ElseIf KeyId > gt.Rows.Count And (gt.Rows(KeyId - count)(1) = Nothing) Then gt.Rows(KeyId - KeyId).Delete() Else If count = 0 Then gt.Rows(KeyId).Delete() Else gt.Rows(KeyId - count).Delete() End If End If count = count + 1 End If
Next GridView1.DataSource = gt GridView1.DataBind()
Revised, this should now work in all cases.
IP: Logged |
And most of your code is still extraneous (and based on incorrect logic). I think its wrong, too.
I think the only reason it might work is because you've added lots of qualifiers to unnecessary statements to make them not trigger except when they're equivalent of what you really need, which is silly.
Btw, what does gt.Rows(KeyId - count)(1) get you?
Posts: 15770 | Registered: Dec 2001
| IP: Logged |
posted
Um, wow. Remember what I said before, Blayne - even with your current scheme all of those if/else constructs are completely unnecessary. It's like adding 2 + 2 like this:
2 + 4 - (3 * 27) + (2 * 51) - 5^2 + 2
You need an If to check if the checkbox is checked. That's it. Everything else is baling wire and duct tape. This whole thing should be 10-15 lines of code, not 30.
Posts: 3275 | Registered: May 2007
| IP: Logged |
Dim count = 0 For Each GridViewRow1 As GridViewRow In GridView1.Rows '---Look at each checkbox here CheckBox1 = CType(GridViewRow1.FindControl("chkSelector"), CheckBox) If CheckBox1.Checked Then gt.Rows(GridViewRow1.RowIndex - count).Delete count = count + 1 End If Next GridView1.DataSource = gt GridView1.DataBind()
posted
You know, a few days ago I was idly paging through Joel's archives, and I came across this article,which contains among much wisdom the words
quote:I know a few superstar programmers who could crank out a Fortran compiler by themselves in one week, and lots of programmers who couldn’t write the code to print the startup banner if they had six months.
For some reason I thought he was exaggerating. I can't imagine what I was thinking. Although cheer up, Blayne, google "Paula Bean" and realise that you haven't yet hit rock bottom.
Posts: 10645 | Registered: Jul 2004
| IP: Logged |
quote:Originally posted by King of Men: You know, a few days ago I was idly paging through Joel's archives, and I came across this article,which contains among much wisdom the words
quote:I know a few superstar programmers who could crank out a Fortran compiler by themselves in one week, and lots of programmers who couldn’t write the code to print the startup banner if they had six months.
For some reason I thought he was exaggerating. I can't imagine what I was thinking. Although cheer up, Blayne, google "Paula Bean" and realise that you haven't yet hit rock bottom.
Thank you for that article. I recently was the applicant and went through a similar interview process at a major medical company in the area, and although it was awkward at the time I now understand the reasoning behind it. And I agree with all of it.
And I've met my share of Paulas...
Posts: 3486 | Registered: Sep 2002
| IP: Logged |
Printer-friendly view of this topic
![[Embarrassed]](redface.gif)
I just had to grandstand, I could have just left it alone but nooooooo my 6th sense just had to force me to double check and no of course I just had to stick to my guns I just couldn't stay quiet, fix my code, I just had to make an ass of myself before realizing I was slightly a smigden mistaken.![[Wink]](wink.gif)
![[Eek!]](eek.gif)
