0

This is my first project in c#. I have a little experience in Access VBA. I would like to move my apps over to be stand alone programs. I'm querying a table that has training types and dates. I would like to compare some of the types of training against each other based on the dates they were performed. The three training types are RWT010, RWP000, and RWT010BP. If RWT010BP exists and is newer it is the only one I need. Otherwise I need RWT010 and RWP000. I have figured out how to load the values into variables, but I need to be able to work with them. I would like the name of the dateTime value to be the trainType for the same row. That way I can compare them and output the right combination.

My old Access logic looked like this:

LABEL_DATE: IIf(IsNull([RWT010]),"RWT010BP: " & _
     Format([RWT010BP],"Short Date"),IIf([RWT010BP]>[RWT010],"RWT010BP: " & _
     Format([RWT010BP],"Short Date"),"RWT010: " & _
     Format([RWT010],"Short Date") & " & " & "RWP000: " & _
     Format([RWP000],"Short Date")))

This is how far I've gotten in c#:

Console.Write("Enter ID: ");
        int idnum = Convert.ToInt32(Console.ReadLine());
        string sql = "SELECT EXPID, TYPE, DATE_LATEST FROM TRAINING_TABLE where expid =" + idnum;



        OracleCommand cmd = new OracleCommand();

       cmd.Connection = conn;

        cmd.CommandText = sql;

        using (DbDataReader reader = cmd.ExecuteReader())
        {
            if (reader.HasRows)
            {

                while (reader.Read())
                {

                    int expid = reader.GetInt32(0);
                    string trainType = reader.GetString(1);
                    DateTime trainDate = reader.GetDateTime(2); 
Tim Williams
  • 154,628
  • 8
  • 97
  • 125
  • Are you saying you want your C# variable name `trainDate` to change according to the value of `trainType`? That would be terrible idea if it were possible, but it isn't. Or do I misunderstand you? – 15ee8f99-57ff-4f92-890c-b56153 Feb 22 '17 at 20:46
  • I guess I really should have asked what would be the most efficient way to do the comparison of the dates of the different training types. I apologize I'm really new at this. – Ory Weaver Feb 23 '17 at 12:47
  • I wouldn't worry about efficiency. Clarity is much more important. My first guess about what's going on here is that in Access, `RWT010` etc. are all date fields in a single row, but in Oracle you have multiple rows, and each row has a name field plus a single date field. And the name field is a string that could be `"RWT010"`, `"RWT010BP"`, or `"RWP000"`, Is that correct? – 15ee8f99-57ff-4f92-890c-b56153 Feb 23 '17 at 14:18

1 Answers1

0

It looks like the original Access logic has one DB row with three date fields, [RWT010], [RWT010BP], and [RWP000]. But in Oracle that's been normalized so you're getting back multiple rows, each of which has a a datetime field named [DATE_LATEST], and then name field called [TYPE] that's equal to "RWT010", "RWT010BP", or "RWP000".

And you were thinking, you want to handle those RWP000 date values by name, just like in Access. You were right, that's the clearest way to do it, and I'll show you how. I misunderstood what you were asking.

One way to do this would be to write an Oracle stored procedure that duplicates the Access logic. That's not the question you asked, but it's a legitimate way to do it. However, it would be more complicated than the Access version due to the change in the database, and anyway I haven't written Oracle SQL in years and I don't have an Oracle server handy to give me arbitrary, cryptic syntax errors about semicolons and whitespace.

So what I'm going to do is write a loop in C# to grab the datetimes from the DB rows and put them in local variables, and then duplicate the Access logic in C# using those variables instead of fields. It'll be a little verbose compared to the Access version, but sometimes that's how it goes.

int idnum = Convert.ToInt32(Console.ReadLine());

string sql = "SELECT EXPID, TYPE, DATE_LATEST FROM TRAINING_TABLE where expid =" + idnum;

//  I don't know how you're using this so I'll just declare it here 
//  and leave that to you.
String dateLabel = "";

OracleCommand cmd = new OracleCommand();

cmd.Connection = conn;

cmd.CommandText = sql;

using (DbDataReader reader = cmd.ExecuteReader())
{
    DateTime? RWT010 = null;
    DateTime? RWT010BP = null;
    DateTime? RWP000 = null;

    //  No need to check reader.HasRows. If it has no rows, reader.Read()
    //  will return false the first time, that's all. 

    while (reader.Read())
    {
        //  Doesn't look to me like expid is used
        //int expid = reader.GetInt32(0);
        string trainType = reader.GetString(1);
        DateTime trainDate = reader.GetDateTime(2);

        switch (trainType) {
            case "RWT010":
                RWT010 = trainDate;
                break;
            case "RWT010BP":
                RWT010BP = trainDate;
                break;
            case "RWP000":
                RWP000 = trainDate;
                break;
        }
    }

    if (RWT010 == null || RWT010BP > RWT010) {
        dateLabel = String.Format("RWT010BP: {0:d}", RWT010BP);
    } else {
        dateLabel = String.Format("RWT010: {0:d} & RWP000: {1:d}", RWT010, RWP000); 
    }
}

The original logic was this:

  If RWT010 isn't null, 
      Do A
  Otherwise, if RWT010BP > RWT010
      ALSO do A
  But if none of the above, 
      Do B

The first two branches do the exact same thing, so we can condense them both into one branch.

"Don't Repeat Yourself", as they say. You don't want to return to this code a year from now, wonder if those two lines were required to be the same, and then guess wrong or else not notice that they are the same, and only change one or the other. It's just a mess.

If you're not familiar with String.Format(), there's a lot to it. In the first argument string, {0} means "insert the second argument here"; {1} means "insert the third", and so on. The ":d" inside the curly braces is optional; it means to pass "d" as format information to the value its inserting. DateTime will interpret that "d" to mean "Short Date". You could also do it like this:

dateLabel = String.Format("RWT010BP: {0}", RWT010BP.Value.ToShortDateString());

Or like this:

dateLabel = "RWT010BP: " + RWT010BP.Value.ToShortDateString();

I have to use RWT010BP.Value in that line instead of just RWT010BP because RWT010BP is declared with a ? after it. That makes it a "nullable" value. A regular DateTime can't be null, but we need to accommodate nulls here.

If you're using C#6, you can do it like this, which I prefer. I didn't use it above because I don't know what version of C# you're on. Always prefer the least amount of "noise" cluttering up the code.

dateLabel = $"RWT010BP: {RWT010BP:d}";

That's the same ":d" as in String.Format("{0:d}", ...) above.

One more thing: idnum is an int, but don't ever concatenate a string value into a SQL string. That's a massive security vulnerability and people here will (rightly, I'm afraid) give you a very hard time for even contemplating it.

Use OracleCommand.Parameters instead, as shown in this answer. I would have used that even in this case, personally, just as a conditioned reflex.

Community
  • 1
  • 1
  • Wow, that's a really good explanation and solution to my code. I got confused about how I got the training types and dates in the first place and then realized I had written the subqueries to handle each type. Good point about the injection vulnerability. There is a lot someone could do with that. – Ory Weaver Feb 24 '17 at 21:14
  • @OryWeaver Thanks. If it resolved the issue, you can mark it as the answer. – 15ee8f99-57ff-4f92-890c-b56153 Feb 25 '17 at 00:18