2
\$\begingroup\$

So I have been programming in VB.net and want to learn C#.

I "successfully" created a simple program that retrieves data from Sql Server during Form1_Load. The program also retrieves data from an MDB file and converts it into a connection string, which I can use throughout the system. I would like to ask for your comments regarding my first time coding in C#.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Data;
using System.Windows.Forms;
using System.Data.SqlClient;
using System.Data.Odbc;
using System.Data.OleDb;



namespace TestInventoryApp
{ 
    public partial class frmForex : Form
    {
        public bool blnFrx = false;
        public bool blnFpass = true;


        public frmForex()
        {
            InitializeComponent();

        }

        static public string connect()
        {
            //I used using because I have read somewhere that 'using' properly disposes an object
            //This method checks if I can connect to sql server using MDBfile
            string strCONNECTIONSTRING;
            string TableName = "BACKUP";
            string constr = @"Provider=Microsoft.Jet.OLEDB.4.0;Data Source=C:\MyProject\bin\system_common.mdb";
            string connstr;
            using (OleDbConnection dbCn = new OleDbConnection(constr))
            using (OleDbCommand dbCo = dbCn.CreateCommand())
            {
                dbCo.CommandText = "SELECT m_TableType.TableType AS TableType,m_TableType.DatabaseSystem AS DatabaseSystem,m_TableType.ConnectionString AS ConnectionString FROM m_table,m_TableType WHERE m_table.TableType = m_TableType.TableType AND m_table.TableName = '" + TableName  +"'";
                dbCn.Open();
                using (OleDbDataReader dbRE = dbCo.ExecuteReader())
                {
                    if (dbRE.Read())
                    {
                        //This line of code gets a single string in the MDB file
                        //Then chops it into parts to make a connection string to sql server
                        strCONNECTIONSTRING = dbRE["ConnectionString"].ToString();
                        strCONNECTIONSTRING = strCONNECTIONSTRING.Substring(27);

                        int hhh = strCONNECTIONSTRING.IndexOf(";");
                        int aaa = strCONNECTIONSTRING.IndexOf(";")+ 1;

                        string dtsrc = strCONNECTIONSTRING.Substring(0, hhh);

                        strCONNECTIONSTRING = strCONNECTIONSTRING.Substring(aaa);
                        strCONNECTIONSTRING = strCONNECTIONSTRING.Substring(4);

                        hhh = strCONNECTIONSTRING.IndexOf(";");
                        aaa = strCONNECTIONSTRING.IndexOf(";")+ 1;

                        string userid = strCONNECTIONSTRING.Substring(0, hhh);

                        strCONNECTIONSTRING = strCONNECTIONSTRING.Substring(aaa);
                        strCONNECTIONSTRING = strCONNECTIONSTRING.Substring(4);

                        hhh = strCONNECTIONSTRING.IndexOf(";");
                        aaa = strCONNECTIONSTRING.IndexOf(";") + 1;

                        string password = strCONNECTIONSTRING.Substring(0, hhh);

                        strCONNECTIONSTRING = strCONNECTIONSTRING.Substring(aaa);
                        strCONNECTIONSTRING = strCONNECTIONSTRING.Substring(9);

                        string initcat = strCONNECTIONSTRING;

                        connstr = "Password=" + password + ";Persist Security Info=True;User ID="+ userid +";Initial Catalog=" + initcat + ";Data Source=" + dtsrc + "";

                        try
                        {
                            SqlConnection conn = new SqlConnection(connstr);
                            conn.Open();
                        }
                        // I'm still figuring out how to use messagebox.show(e.exception) in c#
                        catch (Exception e)
                        {
                            return "";

                        }
                        return connstr;
                    }

                }

            }
            // an error occurs everytime, stating that "Not" all code paths return a value, hence this code
            return "";
        }

        public void sp_FRX_getForexToday(string frxDate, string connected)
        {
            //This sub gets data from sql server
            using (SqlConnection conn = new SqlConnection(connected))
            using (SqlCommand sqlcomm = conn.CreateCommand())
            {
                sqlcomm.CommandText = "dbo.sp_FRX_getFRX";
                sqlcomm.CommandType = CommandType.StoredProcedure;
                sqlcomm.Parameters.AddWithValue("@Date", frxDate);

                using (SqlDataAdapter dta = new SqlDataAdapter(sqlcomm))
                using (DataTable dt = new DataTable())
                {
                    dta.Fill(dt);
                    if (dt.Rows.Count > 0)
                    {
                        txtPHP.Text = dt.Rows[0][0].ToString();
                        txtJPY.Text = dt.Rows[0][1].ToString();
                        txtEUR.Text = dt.Rows[0][2].ToString();
                    }
                    else
                    {
                        txtPHP.Text = "";
                        txtJPY.Text = "";
                        txtEUR.Text = "";
                    }

                }

            }


        }


        private void Form1_Load(object sender, EventArgs e)
        {
            string connected;

            connected = connect();

            if (connected.Length > 0)
            {
                MessageBox.Show("Successful");
            }

            blnFpass = false;
            sp_FRX_getForexToday("2019/06/03", connected);

        }

    }
}

Other things that I would like to learn:

  1. Regarding method connect, I would like to use the connection throughout the system, so I declared a local variable in Form1_Load named connected and put the connection in it. Is this the best practice? In Vb.Net, I could use conn.open and declare Public conn As New SqlClient.SqlConnection then use conn throughout the code. How can I do it in C#?

Thank you for your time and comments.

\$\endgroup\$
  • 1
    \$\begingroup\$ There is a class called OleDbConnectionStringBuilder to help you out. \$\endgroup\$ – dfhwze Jun 8 '19 at 8:36
  • \$\begingroup\$ The question in Other things that I would like to learn almost makes this question off-topic (please see codereview.stackexchange.com/help/dont-ask). I wouldn't store the connection itself, but I would store the connection string. The connection string could be stored in in either the app or the program. \$\endgroup\$ – pacmaninbw Jun 8 '19 at 13:21
  • \$\begingroup\$ @pacmaninbw so,am I correct that I stored the connection string on a method. \$\endgroup\$ – Mr.J Jun 9 '19 at 23:36
  • 1
    \$\begingroup\$ Look up class properties. \$\endgroup\$ – pacmaninbw Jun 10 '19 at 12:10
  • 1
    \$\begingroup\$ Research Entity Framework \$\endgroup\$ – Mark Cilia Vincenti Jun 11 '19 at 11:52
3
\$\begingroup\$

Review

  • order your using statements to get a better overview of imports
  • don't put more than 1 blank line between lines of code, that's just wasting space
  • don't camel case a class name or prefix with an abbreviated type name frmForex; a better name would be ForexForm
  • don't put the type name in variable names and don't use abbrevations blnFrx, blnFpass; pick meaningful names (I have no clue what they mean)
  • prefer properties over fields for public state; but your state should not even be declared public here, so use private bool instead
  • don't use capitals in variable names strCONNECTIONSTRING
  • use an instance of OleDbConnectionStringBuilder to build the connection string for you
  • you are doing good by using using-blocks for IDisposable instances
  • avoid meaningless variable names aaa and hhh
  • don't catch the exception explicitly if you are not going to handle it catch (Exception e) -> catch
  • use var to avoid redundant code var connection = new SqlConnection(connstr);
  • you could use return string.Empty instead of return "";
  • don't snake case method names sp_FRX_getForexToday
\$\endgroup\$
  • 1
    \$\begingroup\$ To add emphasis to a few things you noted: Do not use Hungarian notation. And always give names to your classes, properties, methods, etc that are meaningful and clearly self-documenting (i.e. the name clearly portrays what the thing is, and/or what it does). Avoid abbreviations that are cryptic or not well known. General Naming Conventions \$\endgroup\$ – DBro Aug 29 '19 at 17:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.