Give me strength.


  • Garbage Person

    So I'm going through some reporting code and I've noticed an awful lot of this sort of thing:

     

        Private Sub btnPrintSomeShit_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnPrintSomeShit.Click
    Dim ws As New WS.DBWS
    Dim TableRead As DataTableReader = ws.ExecuteDataset(ConfigurationManager.AppSettings("api_key"), "select categoryID from v_categoriesWithEntriesDetail where metaclass IN (SELECT metaclassID from tbl_Metaclasses WHERE isAThing =0)").CreateDataReader

    Dim contests As New ArrayList()

    While (TableRead.Read())
    Dim contest As Integer = TableRead.GetInt32(0)
    contests.Add(contest)
    End While
    TableRead.Close()


    If (contests.Count <> 0) Then

    Dim contestArr(contests.Count) As Integer
    Dim i As Integer = 0
    For Each thing As Integer In contests.ToArray
    contestArr(i) = Integer.Parse(thing.ToString)
    i = i + 1
    Next

    Dim report As New stupidfuckingreport()
    Dim reportWS As New WS.DBWS
    Dim myDataSet As DataSet = reportWS.ExecuteDataset(ConfigurationManager.AppSettings("api_key"), "select * from v_claimchecks")
    report.Database.Tables("v_claimchecks").SetDataSource(myDataSet.Tables(0))
                report.SetParameterValue("categories", contestArr)
    report.PrintToPrinter(1, False, 0, Integer.MaxValue)
    End If
    End Sub

    Take a look and see how many you can find.



  • @Weng said:

    contestArr(i) = Integer.Parse(thing.ToString)

    I'm not familiar with Basic, but... WHYY???

    Great method of Array handling/conversion btw.


  • Garbage Person

    @derula said:

    I'm not familiar with Basic, but... WHYY???
    I'm fairly certain I could just strip that down to contestArr(i) = thing but I can't be arsed to bother trying.

     @derula said:

    Great method of Array handling/conversion btw.
    Yeah, I especially like how we build an ArrayList, convert it to an array, and then iterate over that array to copy it into a new array.



  • Something tells me that this person got fedup of creating report generators. . .



  • the primary WTF: using two select statements and convulated casting where a single select would've sufficed.



  • I'll point out the gross lack of  word wrap on line three.



  • .. also people who insist on using unnecessary parentheses in If starments in VB.



  • @Cantabrigian said:

    .. also people who insist on using unnecessary parentheses in If starments in VB.
     

    I kind of defend that in that it's harmless.  Having written C and kin for so long, naked flow conditions feel spooky.

     



  • @arty said:

    @Cantabrigian said:
    .. also people who insist on using unnecessary parentheses in If starments in VB.
    I kind of defend that in that it's harmless.  Having written C and kin for so long, naked flow conditions feel spooky.
    I feel compelled to second this sentiment. It's quite nice to have paren delimited logic so that the intent is without misunderstanding. Better something like this @contrived example of paren code said:
    ( (salesTotal<>0) and ( (true>1) >0 ) )
    regardless how screwed up the internal logic is. Now during code review you would immediately question the INTENT of the second half of my contrived example (which should work out to false, thus always evaluating to false for the expression...)

    also, am I the only one who felt like I was reading C code in VB?

    So I'm going through some reporting code and I've noticed an awful lot of this sort of thing:

        Private Sub btnPrintSomeShit_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnPrintSomeShit.Click
    <font color="#ff0000">Dim ws As New WS.DBWS</font>
    Dim TableRead As DataTableReader = ws.ExecuteDataset(ConfigurationManager.AppSettings("api_key"), _
    "select categoryID from v_categoriesWithEntriesDetail where metaclass IN _
    (SELECT metaclassID from tbl_Metaclasses WHERE isAThing =0)").CreateDataReader

    Dim contests As New ArrayList()

    While (TableRead.Read())
    <font color="#ff0000"> Dim contest As Integer</font> = TableRead.GetInt32(0)
    contests.Add(<font color="#ff00ff">contest</font>)
    End While
    TableRead.Close()


    If (contests.Count <> 0) Then

    Dim contestArr(contests.Count) As Integer
    Dim i As Integer = 0
    For Each thing As Integer In contests.ToArray
    contestArr(i) = Integer.Parse(thing.ToString)
    i = i + 1
    Next

    Dim report As New stupidfuckingreport()
    <font color="#ff0000">Dim reportWS As New WS.DBWS</font>
    Dim myDataSet As DataSet = _
    reportWS.ExecuteDataset(ConfigurationManager.AppSettings("api_key"), "select * from v_claimchecks")
    report.Database.Tables("v_claimchecks").SetDataSource(myDataSet.Tables(0))
                report.SetParameterValue("categories", contestArr)
    report.PrintToPrinter(1, False, 0, Integer.MaxValue)
    End If
    End Sub

    Take a look and see how many you can find.

    RED are needless recasts, purple is he could've just put the damned read call in here.

    So without any heavy profiling or actually trying to make sure this is optimal, I would make the following changes...

        Private Sub btnPrintSomeShit_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnPrintSomeShit.Click
    <font color="#000000">Dim reportWS As New WS.DBWS</font>
    Dim TableRead As DataTableReader = reportWS.ExecuteDataset(ConfigurationManager.AppSettings("api_key"), _
    "select categoryID from v_categoriesWithEntriesDetail where metaclass IN _
    (SELECT metaclassID from tbl_Metaclasses WHERE isAThing =0)").CreateDataReader

    Dim contests As New ArrayList()

    While (TableRead.Read())
    contests.Add(TableRead.GetInt32(0))
    End While
    TableRead.Close()

    If (contests.Count <> 0) Then
    Dim report As New stupidfuckingreport()
    Dim myDataSet As DataSet = _
    reportWS.ExecuteDataset(ConfigurationManager.AppSettings("api_key"), "select * from v_claimchecks")
    report.Database.Tables("v_claimchecks").SetDataSource(myDataSet.Tables(0))
                report.SetParameterValue("categories", contestArr)
    report.PrintToPrinter(1, False, 0, Integer.MaxValue)
    End If
    End Sub

    However, now that we've gotten it down to functional logic and the things that are extraneous being gone, it looks like a single query getting fed into a single report. Seems like there's a way to optimize the query and get this even faster, but I don't know the schema, so ....

     

    PS: for those blessed with never using VB, the _ character means "the rest of this statement follows on the next line, nothing else comes after the _" unless it's surrounded by two ascii characters (ie a variable name)

    								    </div>
    								    
    								    </div>


  • @drachenstern said:

    Dim contests As New ArrayList()

    Much better would be List(Of Integer). This has better performance as ArrayList would box each integer.

    @drachenstern said:

    PS: for those blessed with never using VB, the _ character means "the rest of this statement follows on the next line, nothing else comes after the _" unless it's surrounded by two ascii characters (ie a variable name)

    Apparently the next version of VB does not need the _ any more.



  • @SlyEcho said:

    @drachenstern said:
    So without any <font color="#999999">heavy profiling or</font> actually trying to make sure this is optimal,
    Dim contests As New ArrayList()

    Much better would be List(Of Integer). This has better performance as ArrayList would box each integer.

    I think that was the original intent anyways (not that it matters). All I really did was try to cut out the redundant parts that weren't necessary. Besides, I personally would've just gone for a more highly tuned query, but ...

    @SlyEcho said:
    @drachenstern said:
    PS: for those blessed with never using VB, the _ character means "the rest of this statement follows on the next line, nothing else comes after the _" unless it's surrounded by two ascii characters (ie a variable name)

    Apparently the next version of VB does not need the _ any more.

    Eh what's that? (sorry that my brain didn't grok "line continuation" when I was trying to explain the underscore earlier) I guess you mean something like this article here? Wow, that won't be misused and mistaken ... sure.



  • @drachenstern said:

    @arty said:
    @Cantabrigian said:
    .. also people who insist on using unnecessary parentheses in If starments in VB.
    I kind of defend that in that it's harmless.  Having written C and kin for so long, naked flow conditions feel spooky.
    I feel compelled to second this sentiment. It's quite nice to have paren delimited logic so that the intent is without misunderstanding. Better something like this @contrived example of paren code said:
    ( (salesTotal<>0) and ( (true>1) >0 ) )
    regardless how screwed up the internal logic is.

    Agreed.  My in-brain parser likes statements that leave no ambiguity about the intent.  Also, it's easier to divine the meaning of nested parens at-a-glance versus having to read the entire statement and decipher the order of operations.


  • Garbage Person

    @Mole said:

    Something tells me that this person got fedup of creating report generators. . .
    This entire (Winforms) application consists of primarily buttons that tie into a method that looks prettymuch exactly this one. Details vary - some pull the parameters right out of a table (like this one), some pull parameters out of other GUI controls, some pull parameters out of an RNG (no, really).

     

     But in general every single one of them follows the same format:

     

    {make a web service call or divine some parameters from some other means}
    {stick those parameters into an ArrayList}
    {Convert the ArrayList to an array of objects}
    {Iterate over that array of objects and shove its contents into an array of Integers}


    {Make another web service call to get the raw data table to pass to CrystalReports - full table dumps every time, regardless of what the report actually needs}
    {Feed the report the parameters and data table}
    {Do something with the report - either print it or shove it into a reportviewer window}

     

    So they DEFINITELY didn't get bored writing the same generator over and over again - they just did it poorly the first time out and copypasted it about 90 times.

     

     

     

    Also, re: line continuation. This was originally coded on workstations with 17" LCDs running 1024x768 - so I have NO idea how they survived. The gigantic lines don't bother me, though - 3840 horizontal pixels will do that.


  • Garbage Person

    @zipfruder said:

    the primary WTF: using two select statements and convulated casting where a single select would've sufficed.
    Actually no, because this same report file is accessed manually - so we can't just blend the parameters into the data table select.Well, we can, but we still have to provide the parameters anyway.


  • :belt_onion:

    @Weng said:

    @Mole said:
    Something tells me that this person got fedup of creating report generators. . .
    This entire (Winforms) application consists of primarily buttons that tie into a method that looks prettymuch exactly this one. Details vary - some pull the parameters right out of a table (like this one), some pull parameters out of other GUI controls, some pull parameters out of an RNG (no, really).
    Needs more MVP for crying out loud. Eventhandler offloads the work to the controller after the view has saved the user input to the model. Controller executes the webservice calls and calls back into the view to update the screen... I mean... view!


  • Garbage Person

    @bjolling said:

    @Weng said:

    @Mole said:
    Something tells me that this person got fedup of creating report generators. . .
    This entire (Winforms) application consists of primarily buttons that tie into a method that looks prettymuch exactly this one. Details vary - some pull the parameters right out of a table (like this one), some pull parameters out of other GUI controls, some pull parameters out of an RNG (no, really).
    Needs more MVP for crying out loud. Eventhandler offloads the work to the controller after the view has saved the user input to the model. Controller executes the webservice calls and calls back into the view to update the screen... I mean... view!

    Because what we all REALLY need in life is an extra layer of binary in some idiot executive's "I WANT TO CLICK BUTTONS TO GET REPORTS!" program... And TECHNICALLY speaking, we already do that anyway because the web service in the middle can act as controller.

  • :belt_onion:

    @Weng said:

    @bjolling said:

    @Weng said:

    @Mole said:
    Something tells me that this person got fedup of creating report generators. . .
    This entire (Winforms) application consists of primarily buttons that tie into a method that looks prettymuch exactly this one. Details vary - some pull the parameters right out of a table (like this one), some pull parameters out of other GUI controls, some pull parameters out of an RNG (no, really).
    Needs more MVP for crying out loud. Eventhandler offloads the work to the controller after the view has saved the user input to the model. Controller executes the webservice calls and calls back into the view to update the screen... I mean... view!

    Because what we all REALLY need in life is an extra layer of binary in some idiot executive's "I WANT TO CLICK BUTTONS TO GET REPORTS!" program... And TECHNICALLY speaking, we already do that anyway because the web service in the middle can act as controller.
    Yes but then you need to add an anti-corruption layer in between because your client application is sharing the domain entities of your webservice which is not a very SOA thing to do.


  • @bjolling said:

    ...your client application is sharing the domain entities of your webservice which is not a very SOA thing to do.

     

    Says who?  As long as they're in a separate assembly, your contract is valid and your abstraction still holds.  WCF even encourages this with its type-sharing system.


  • :belt_onion:

    @Aaron said:

    @bjolling said:

    ...your client application is sharing the domain entities of your webservice which is not a very SOA thing to do.

     

    Says who?  As long as they're in a separate assembly, your contract is valid and your abstraction still holds.  WCF even encourages this with its type-sharing system.

    Says Eric Evans and his unified elephant.

    You have the domain of the webservice behind the WCF and you have the domain of the client application. The types you share are the DTO objects (Date Transfer Objects). Your client wouldn't work directly with these DTOs but instead have a small layer that translates the DTOs into the domain objects of the client. This layer is called the anti-corruption layer. You use it to protect your client's domain from changes in the service domain.

     Sorry, didn't mean to drag this conversation into an architectural debate. Just wanted to indicate that the OP's application could have been far more complicated and that he should count himself lucky the original coder hadn't heard of domain driven design and the MVC pattern


  • Garbage Person

    @bjolling said:

     Sorry, didn't mean to drag this conversation into an architectural debate. Just wanted to indicate that the OP's application could have been far more complicated and that he should count himself lucky the original coder hadn't heard of domain driven design and the MVC pattern
    I penned the original code and architecture for this program. None of the code survived, but the KISS-and-get-back-to-doing-something-useful architecture carried on (actually, almost - the web service replaces direct calls to the database)


Log in to reply