Not exactly code review or source code control

  • Hi

    At our shop, code review entails that certain features are always present in a query going into production so things like a stored procedure header are present, a try catch block is present, that no count is present etc...granted these are kind of mechanical, but we check them anyway because we value them being there.

    Similarly, developers use source code control to check their work in and out.

    I start with these because my problem is somehting else, because these two steps were followed prior to installing the sprocs on the SSRS dedicated server.

    The problem arising is that during code review, the sense of the query is not checked, because the jr dba does not have command of all the business processes at hand; for example, there are considerations pertaining to how to determine continuous enrollment, how to determine the total amount paid versus any denied or pended claim lines, how to determine which site a visit occurred. In the case that sparked this question, it was how to determine the number of Emergency Room visits.

    Two developers created queries that passed code review, yet, they yielded different answers. One practitioner coded the query with a where clause that specified a place of service, a provider type and a date range. The other practioner coded the query with a where clause that specified the same place of service, the same date range, but instead of provider type, they included a provider id instead.

    The issue arising is that the provider type query yielded the expected ER visit number, or at least the intended number of ER visits because the result was collapsed around provider type, masking the possiblity of more than one provider present on the claim.

    The second query yielded a higher number of ER visits because of the way the claim was paid, artificially inflating the number of visits because the fact of the matter is that one member visitied the ER once on that day.

    The problem is that both could be considered correct depending on the context or intent of the report requestor, depending on whether they wanted to evaluate the number of ER visits, or the accuracy of claims processing. Only one provider should have been paid in any case, but the second query isolating provider id picked up the claims department's handling of the claim payment, while the first query masked that mutliplicty and returned the expected result.

    My question is, is there a way to state the requirements for a report in an unambiguous way so that competing formualtions of the query do not happen? if we knew how to do that, it would become part of the code review process.

    thanks very much

    drew

  • drew.georgopulos (5/23/2012)


    Two developers created queries that passed code review, yet, they yielded different answers. One practitioner coded the query with a where clause that specified a place of service, a provider type and a date range. The other practioner coded the query with a where clause that specified the same place of service, the same date range, but instead of provider type, they included a provider id instead.

    How about stating the filters of the report? 🙂 In this case you have 1 by Place of Service, Provider Type, and Date Range. The second is Place of Service, Provider Id, and Date range. Those are 2 very different things and can easily be placed into a requirements doc. Not really sure what you are asking because the requirements are different for each. If the doc is not this specific, it really should be.

    Jared
    CE - Microsoft

  • How do you unambiguoulsy define your requirements, with language or diagrams?

    Are your DBAS masters of your business' business rules?

  • My standard for documentation, in the code (comments) or in the project (ticketing system discussion thread), is that you have to document WHY some piece of code is the way that it is.

    If there are two ways to determine the providers (by ID or by type), document why you chose the one you did. Preferably with a comment in the stored procedure.

    It's an ideal way to capture business-rules close to where they are implemented, and it makes "odd looking" queries much easier to understand after time has passed. Has the added benefit that, if a query is a particular way for a valid reason, and that's documented right there, someone working on a refactor will generally clarify if business rules are still valid, rather than just going "that can't be right - what was that idiot thinking?" and changing something that actually shouldn't be changed. Protects the rules the same way source control protects the code.

    - Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
    Property of The Thread

    "Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon

  • How good in the communication between the end-users of the reports, and the developers, who can develope the query correctly only if they completely understand what whe end-users are seeking?

    In the example you provided, it seems very likely to me that the initial request might not contain sufficient information for the developer to produce a query with the desired result, since the report end-user may not understand the challenges query writing, and the developer may not fully understand the requirements of claims proccessing. I would think some back & forth would be required to get it right.

  • drew.georgopulos (5/23/2012)


    How do you unambiguoulsy define your requirements, with language or diagrams?

    Are your DBAS masters of your business' business rules?

    In my previous company, the DBA's were also the SQL developers and project managers and business analysts and data architects... lol So, yes. That being said, we still did not always know what the stakeholders wanted. We certainly did not assume or guess. Because I have a statistics background, I would make suggestions and describe the difference in interpretation of those results. Most of the times, I just asked.

    Requirements documents should always be provided for reports. If not a full spec, at least the specific assumptions of the report. A report is always answering a question, so I would always start with that... "What question are you trying to answer?" In this case, the question was too vague "how to determine the number of Emergency Room visits" Then it is the report writer/project manager's job to ask "What does that mean? Does it mean how many in a day? How many by 1 person?" It is not the stakeholders' job to make sure the requirements are understood, it is the developer's job to make sure that they understand the requirements. Sometimes they are defined by language, sometimes diagrams, and sometimes both.

    On another note, my current company does not have DBAs writing reports. That is the job of our reporting team. So, I am no longer a master of the business rules. However, when I do a code review and optimize their SQL I ask lots and lots of questions. "This is what the code is doing. Is that what you want it to do? Are you sure you want this average, because the count you are getting contains duplicates." etc.

    Jared
    CE - Microsoft

  • GSquared (5/23/2012)


    My standard for documentation, in the code (comments) or in the project (ticketing system discussion thread), is that you have to document WHY some piece of code is the way that it is.

    Thanks very much. you have struck on the missing ingredient in our proc headers.

    drew

  • not so good...there are business analysts interposed between the report requestor and developers, so developers rarely speak to the end user.

    My feeling is that GSquared's suggestion hits the nail on the head, we need to doc the WHY it is the way it is in the header to provide the reason things were decided they way they were. It's death for the CMO to say 'this can't be correct'

    thanks very much

    drew

  • i was hoping for a less interactive/intensive way to arrive at a statement of what exactly the problem we are trying to solve is, but you are right, i guess there is no substitute for asking a lot of questions.

    my preference though would be to get them asked and documented up front by the requirements gatherers instead of later by the technicians and include the analysts documents with the code review so we know what the problem we're trying to solve is in addition to its optimization.

    thanks very much

    drew

  • drew.georgopulos (5/23/2012)


    not so good...there are business analysts interposed between the report requestor and developers, so developers rarely speak to the end user.

    My feeling is that GSquared's suggestion hits the nail on the head, we need to doc the WHY it is the way it is in the header to provide the reason things were decided they way they were. It's death for the CMO to say 'this can't be correct'

    thanks very much

    drew

    Just keep in mind that documenting something that is wrong does not make it right :hehe: I have documented why something was done, and still been yelled at: "I don't care why the code was written that way, why did you think that the code was supposed to be written that way?" In your case, whoever puts together the requirements is the one who should detail the assumptions. In this case, it just seems so simple to clarify providerType versus ProviderId. That is certainly something that can be written into a requirements doc and understood by the end-user, requestor, analyst, and developer. Good luck to you!

    Jared
    CE - Microsoft

  • Code can be impeccably reviewed, verified to be syntaxully correct, efficiently optimized and thoroughly documented, but if does not return exectly what the end user needs, it is still wrong.

  • drew.georgopulos (5/23/2012)


    i was hoping for a less interactive/intensive way to arrive at a statement of what exactly the problem we are trying to solve is, but you are right, i guess there is no substitute for asking a lot of questions.

    my preference though would be to get them asked and documented up front by the requirements gatherers instead of later by the technicians and include the analysts documents with the code review so we know what the problem we're trying to solve is in addition to its optimization.

    thanks very much

    drew

    There are ALWAYS unforseen developments regarding the business rules in the development process. If you don't see any, you're probably just not looking hard enough.

    It's very, very rare for a project I've worked on to be "requirements complete" without some back and forth during the dev process with the stakeholder and the dev (or, inferiorly, with an intermediary).

    English (or any other language except pure math) is ambigous by design, so it's pretty much impossible to have a non-interactive method of arriving at exact requirements and exact code. Well, unless the business stakeholder IS the developer, of course. Then it's intra-active instead of inter-active, but it still works the same way.

    - Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
    Property of The Thread

    "Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon

  • drew.georgopulos (5/23/2012)


    Are your DBAS masters of your business' business rules?

    In my office, we unfortunately know more about the business rules than our business users do. Additionally, there are too few DBAs to do such a comprehensive code evaluation and approval as what you seek.

    Fortunately, we have a stellar QA / Testing team that rigorously tests (along with the business unit) every bit of code that goes into our environment before it hits production. Since the BU has to provide business cases for the testing, and expected results, this helps us find holes and problems in the code. Or, as in your stated example, differences between what our requirements said and what the dev was thinking.

    Requirements include business rules (expected final behavior), filters (if needed), and calculations (listed with BU language). They don't tell us what tables / columns to use, they don't even define how the database schema is to be updated, altered, or used. However, they do break down how things should look at the end of the day.

    Brandie Tarvin, MCITP Database AdministratorLiveJournal Blog: http://brandietarvin.livejournal.com/[/url]On LinkedIn!, Google+, and Twitter.Freelance Writer: ShadowrunLatchkeys: Nevermore, Latchkeys: The Bootleg War, and Latchkeys: Roscoes in the Night are now available on Nook and Kindle.

  • SQLKnowItAll (5/23/2012)


    drew.georgopulos (5/23/2012)


    not so good...there are business analysts interposed between the report requestor and developers, so developers rarely speak to the end user.

    My feeling is that GSquared's suggestion hits the nail on the head, we need to doc the WHY it is the way it is in the header to provide the reason things were decided they way they were. It's death for the CMO to say 'this can't be correct'

    thanks very much

    drew

    Just keep in mind that documenting something that is wrong does not make it right :hehe: I have documented why something was done, and still been yelled at: "I don't care why the code was written that way, why did you think that the code was supposed to be written that way?" In your case, whoever puts together the requirements is the one who should detail the assumptions. In this case, it just seems so simple to clarify providerType versus ProviderId. That is certainly something that can be written into a requirements doc and understood by the end-user, requestor, analyst, and developer. Good luck to you!

    We ask our BA's to provider requirements AND acceptance tests (if I put this in, I get this out; if I put this OTHER thing in, I get an error of type xx, etc...). They've come to understand that if the code "passes" their test, it's what they asked for.

    ----------------------------------------------------------------------------------
    Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?

  • SQLKnowItAll (5/23/2012)


    drew.georgopulos (5/23/2012)


    not so good...there are business analysts interposed between the report requestor and developers, so developers rarely speak to the end user.

    My feeling is that GSquared's suggestion hits the nail on the head, we need to doc the WHY it is the way it is in the header to provide the reason things were decided they way they were. It's death for the CMO to say 'this can't be correct'

    thanks very much

    drew

    Just keep in mind that documenting something that is wrong does not make it right :hehe: I have documented why something was done, and still been yelled at: "I don't care why the code was written that way, why did you think that the code was supposed to be written that way?" In your case, whoever puts together the requirements is the one who should detail the assumptions. In this case, it just seems so simple to clarify providerType versus ProviderId. That is certainly something that can be written into a requirements doc and understood by the end-user, requestor, analyst, and developer. Good luck to you!

    If the documentation is for CYA, which is what this sounds like, then it's useless. The guy with hire/fire authority is always right in that kind of situation. That's why I don't work places where I need to protect myself that way.

    - Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
    Property of The Thread

    "Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon

Viewing 15 posts - 1 through 15 (of 16 total)

You must be logged in to reply to this topic. Login to reply