1
\$\begingroup\$

I am a newbie in C# and in OOP in general. I decided to create a class that should give out a "connection" object. I would like your advice and critique on improving it.

public class Connection_Factory
{

  private string Last_Connection_Trying   = "";
  private string Last_Connection_Success = "";
  public OracleConnection Build(string connection_string)
  {
    Last_Connection_Trying = connection_string;
    return new OracleConnection(connection_string);
  }

  public OracleConnection Build()
  {
    return Build(Last_Connection_Success);
  }

  public void Mark_Connection_as_Successful(string connection_string)
  {
    Last_Connection_Success = connection_string;
  }

  public void Mark_Connection_as_Successful()
  {
    Last_Connection_Success = Last_Connection_Trying;
  }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ What are you trying to achieve? What is the purpose of marking a connection as successful? This factory does not make any sense. \$\endgroup\$ – t3chb0t Mar 31 '17 at 6:27
  • \$\begingroup\$ For example, user is trying to connect to Server via Login Form. If connection was successful then you mark connection as successful and futher you can create a connections without neccessity of repeated specifying a connection string again. And in general I guess that creating a connection object should be in one point in application. \$\endgroup\$ – Gordon Freeman Mar 31 '17 at 6:32
  • \$\begingroup\$ Do you have more then one connection string or why would you want to avoid specifying it multiple times or rather why should you need to specify it more then once? It's not like it changes all the time, is it? \$\endgroup\$ – t3chb0t Mar 31 '17 at 8:56
  • \$\begingroup\$ yeah, I can have more than one database, let's say test DB and production DB. \$\endgroup\$ – Gordon Freeman Mar 31 '17 at 9:00
  • 1
    \$\begingroup\$ @GordonFreeman But the client code specifies the connection string the first time. So why shouldn't it be expected to continue to do so? And in fact the client either has to continue to supply a connection string when calling Build, or knowingly switch to the parameterless overload. This imo is too much knowledge of the factory's internals. \$\endgroup\$ – 404 Mar 31 '17 at 9:21
1
\$\begingroup\$

Firstly your code lacks any comments saying what it does and why. When I first looked at your post I assumed it was a TCP socket class and was looking for the port number. Also the layout could be improved, regular blank lines, etc.

I would do this:

public void Mark_Connection_as_Successful()
{
    Mark_Connection_as_Successful(Last_Connection_Trying);
}

just like you have with Build. At the moment they are trivial functions, but that might not always be the case.

Would it be appropriate to verify the format of the connection string, or is this done in the OracleConnection constructor?

\$\endgroup\$
  • \$\begingroup\$ Thank you, Matt, I agree with you. And I think that verifying connection string format is a responsibility of OracleConnection constructor. \$\endgroup\$ – Gordon Freeman Mar 31 '17 at 12:57
  • 3
    \$\begingroup\$ Comments should say why, not what... the code should say what all by itself ;-) \$\endgroup\$ – Mathieu Guindon Mar 31 '17 at 13:20
  • \$\begingroup\$ I never remember which one its meant to be, its not like I write any :) \$\endgroup\$ – Code Gorilla Mar 31 '17 at 14:14

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.