Are parameterized C# constructors that do more than create the instance considered bad practice?

Hi all,

I've heard that C# class constructors with parameters that actually DO something are bad practice.  Is that true?  For example, please see the code snippet below.  As you can see there is a class called LoggingService with a constructor that accepts 2 parameters then writes to a log file based on those parameters.

Is this bad practice?  If so, why?  I want to try and minimise the code I need to perform simple logging operations but this way looks weird as people using the class wouldn't know the constructor is writing to the log file.  Should I always have a constructor that does nothing (and thereby implied) and have a method called, say, LogMessage that does the actual logging?

I figured that having the constructor doing the logging would mean less code as it means creating the instance of the class and then not having to write more code that calls another method that does the actual logging.

Am I making sense?  Haha ... sorry if I'm not.  abel?  :)

Thanks in advance!
public class LoggingService
{
  public LoggingService(string logFile, string message)
  {
    TextWriter tw = new StreamWriter(logFile, true);
    tw.WriteLine(String.Format("[{0}] - {1}", DateTime.Now, message));
    tw.Close();
  }
}

Open in new window

LVL 3
Number5ixAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

Daniel WilsonCommented:
A lot of this is opinion, but here is mine, along with my reasons.

Having a constructor with parameters to set things up is fairly standard.  I would include the logFile parameter in the constructor.

You will want a method to write the message(s).  I would not put that initial write in the constructor.

But ... if I DID put the write in the constructor, i would actually have the constructor call my Write method so that my actual file I/O was in only 1 method.

Code reuse is one of the main reasons for OOP, after all.

But the bigger reason is clarity of code.  Will a consumer of the LoggingService expect the constructor to log that message?  Just it being a constructor doesn't tell the consumer about that.  Which is why I would not put the initial write in the constructor.

Writing one more line of code when you use the service won't cause bugs.  Not realizing what the service does likely will.
0
GuitarRichCommented:
To do that I would probably use a static method rather than in the constructor to do it. Or if you do make sure you have a blank constructor so that it doesn't slow down any other processes that create that object.
0
openshacCommented:
Yeah it would better to have a separate method to begin the logging.

I figured that having the constructor doing the logging would mean less code as it means creating the instance of the class and then having more code that calls another method that does the actual logging.

Think about the SqlConnection object in C#, after you create the connection you need to call the Open() method.  Now I don't see lots of people complaining that there is this unneccessary method which makes their code verbose.  In fact it offers alot more functionality and allows the developer to control their resources more accurately.

I would suggest the there would be benefits to allowing developers to initialise your logging class before starting the actual logging.  Remember you are working with unmanaged resources here.

Should I always have a constructor that does nothing (and thereby implied) and have a method called, say, LogMessage that does the actual logging?

Yes, I think this would be best in this case.
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
Cloud Class® Course: Microsoft Exchange Server

The MCTS: Microsoft Exchange Server 2010 certification validates your skills in supporting the maintenance and administration of the Exchange servers in an enterprise environment. Learn everything you need to know with this course.

Number5ixAuthor Commented:
Hi DanielWilson and GuitarRich,

Thanks for your answers.  Daniel you might remember a question I posted recently that blew out into a lengthy discussion about when to use class and instance methods.

Anyway, this one follows on from that as I did have the logging method static but gained from the previous question that having class methods should be avoided unless there's a very good reason for it (one of the posters said something to that effect).

Clarity of code is something I considered and for that reason I currently have the logging method as an instance method so that I can maintain state (more than 1 method may be logging at the same time and for that I believe I *need* state).  Is that right?
0
Number5ixAuthor Commented:
Hi openshac,

Great answer, I think that gives me what I was going for.  The SqlConnection example explained it pretty well.  :)

Thanks again
0
GuitarRichCommented:
> Should I always have a constructor that does nothing (and thereby implied) and have a method called, say, LogMessage that does the actual logging?

The constructor should do exactly what is says - construct the object - so it is used for initialising objects - basically getting the object ready to be used. If you want minimal code - then a static method would be best - but as openshac said, allowing developers to initialise things before using your logging class could help them e.g there is nothing wrong with having the logfile name passed to the constructor and storing it in a variable for later use. Infact that would cut down on code too because you wouldn't have to pass in the logfile name every time. Like this:


public class LoggingService
{
  private string _logFile;
 
  public LoggingService(string logFile)
  {
    _logFile = logFile;
  }
 
  public void LogMessage(string message)
  {
    TextWriter tw = new StreamWriter(_logFile, true);
    tw.WriteLine(String.Format("[{0}] - {1}", DateTime.Now, message));
    tw.Close();
  }
}

Open in new window

0
Number5ixAuthor Commented:
Excellent answer, thanks openshac.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
C#

From novice to tech pro — start learning today.