4
\$\begingroup\$

I'd like to have this code looked over because I feel my approach might be very novice and there has to be a more elegant or at least less "grunt-work" way of doing it.

I have to write a small TCP client in my application which can send an object to a server for it to be stored in a Database. Before I send the object I remember that I was once told sending custom objects is generally bad practice unless you have a custom Stream Class (Which I don't have). So I am dissecting my objects before I send them into their respective Strings/ints/whatever else objects. I just feel my approach might not be very efficient.

The end result will be that the server can put together a Query to be executed in the database for storage.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Smart_Journal.Familie;
using System.Net.Sockets;
using System.IO;

namespace Smart_Journal
{
    /// <summary>
    /// DBM is our Database Manager. It will handle sending and receiving data from the server.
    /// You should NOT call these functions directly. Instead, call the functions in the Utility Class.
    /// </summary>
    public static class DBM
    {
        private static TcpClient _client;
        private static StreamReader _sReader;
        private static StreamWriter _sWriter;
        private static bool _isConnected;
        private static int portNum;
        private static String ipAddress;

        private static void InitConnection()
        {
            _client = new TcpClient();
            _client.Connect(ipAddress, portNum);

            _sReader = new StreamReader(_client.GetStream(), Encoding.ASCII);
            _sWriter = new StreamWriter(_client.GetStream(), Encoding.ASCII);

            _isConnected = true;
        }

        private static void EndConnection()
        {
            _sWriter.WriteLine("END");
            _sWriter.Flush();
            _sReader.Close();
            _sWriter.Close();
            _client.Close();
            _isConnected = false;
        }
        // Basically means: SendSupportFamilyObject
        public static void SendPlejeFamilieObjekt(PlejeFamilie pfamilie)
        {
            InitConnection();
            if (_isConnected)
            {
                _sWriter.WriteLine("SEND_PLEJEFAMILIE");
                _sWriter.Flush();
                if (_sReader.ReadLine().Equals("AUTH_GIVEN"))
                {
                    #region Send Plejefamilie Objekt
                    _sWriter.WriteLine(pfamilie.CPRKvinde);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.CPRMand);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.NavnKvinde);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.NavnMand);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.Addresse);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.MobilKvinde);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.MobilMand);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.HjemmeTelefon);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.StillingKvinde);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.StillingMand);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.EmailKvinde);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.EmailMand);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.EmailFaelles);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.GodkendtDato);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.PlejeForaeldreUdd);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.AntalKurserIAar);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.AntalBoernGodkendt);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.Vilkaar);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pfamilie.Status);
                    _sWriter.Flush();
                    #endregion Send Plejefamilie Objekt
                }
            }
            EndConnection();
        }
        // Basically means: SendSpecialNeedsChildObject
        public static void SendPlejeBarnObjekt(PlejeBarn pbarn)
        {
            InitConnection();
            if (_isConnected)
            {
                _sWriter.WriteLine("SEND_PLEJEBARN");
                _sWriter.Flush();
                if (_sReader.ReadLine().Equals("AUTH_GIVEN"))
                {
                    #region Send Plejebarn Objekt
                    _sWriter.WriteLine(pbarn.CPR);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.Navn);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.FolkeregisterAdresse);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.Email);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.Telefon);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.Sagsbehandler);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.Konsulent);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.Aflastning);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.NuvaerendeForanstaltning);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.AnbringelsesDato);
                    _sWriter.Flush();
                    _sWriter.WriteLine(pbarn.UdskrivningsDato);
                    _sWriter.Flush();
                    _sWriter.WriteLine("PLEJEBARN_B");
                    _sWriter.Flush();
                    foreach(Note n in pbarn.Bemaerkninger)
                    {
                        _sWriter.WriteLine(n.Dato);
                        _sWriter.Flush();
                        _sWriter.WriteLine(n.Navn);
                        _sWriter.Flush();
                        _sWriter.WriteLine(n.Tekst);
                        _sWriter.Flush();
                    }
                    _sWriter.WriteLine("END_PLEJEBARN_B");
                    _sWriter.Flush();
                    _sWriter.WriteLine("PLEJEBARN_TF");
                    _sWriter.Flush();
                    foreach (Note n in pbarn.TideligereForanstaltninger)
                    {
                        _sWriter.WriteLine(n.Dato);
                        _sWriter.Flush();
                        _sWriter.WriteLine(n.Navn);
                        _sWriter.Flush();
                        _sWriter.WriteLine(n.Tekst);
                        _sWriter.Flush();
                    }
                    _sWriter.WriteLine("END_PLEJEBARN_TF");
                    #endregion Send Plejebarn Objekt
                }
            }
            EndConnection();
        }
        // Basically means: SendBiologicalFamilyObject
        public static void SendBiologiskFamilieObjekt(BiologiskFamilie bfamilie)
        {
            InitConnection();
            if (_isConnected)
            {
                if (_sReader.ReadLine().Equals("AUTH_GIVEN"))
                {
                    #region Send Biologiskfamilie Objekt
                    _sWriter.WriteLine(bfamilie.CPRMor);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.CPRFar);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.NavnMor);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.NavnFar);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.Addresse);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.MobilMor);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.MobilFar);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.HjemmeTelefon);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.EmailMor);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.EmailFar);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.EmailFaelles);
                    _sWriter.Flush();
                    _sWriter.WriteLine(bfamilie.ForaeldreMyndighed);
                    _sWriter.Flush();
                    #endregion Send Biologiskfamilie Objekt
                }
            }
            EndConnection();
        }

        public static PlejeFamilie HentPlejeFamilieObjekt(String s)
        {
            return null;
        }

        public static PlejeBarn HentPlejeBarnObjekt(String s)
        {
            return null;
        }

        public static BiologiskFamilie HentBiologiskFamilie(String s)
        {
            return null;
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ are you worried about thread safety or not because currently it's not thread safe \$\endgroup\$ – ratchet freak Nov 27 '13 at 15:07
  • \$\begingroup\$ @ratchetfreak A client should only send one object at a time so it shouldn't be an issue, right? The server that it sends to will be able to handle multiple clients at once however. \$\endgroup\$ – OmniOwl Nov 27 '13 at 15:10
  • \$\begingroup\$ I know this is asking a lot but _sWriter.WriteLine(pfamilie.CPRKvinde); can you translate everything to English? if not that is cool. it just helps to understand what is really going where. \$\endgroup\$ – Malachi Nov 27 '13 at 15:10
  • \$\begingroup\$ if you have multiple thread somewhere it may be possible that you need thread safety on this, at least document it as not thread safe \$\endgroup\$ – ratchet freak Nov 27 '13 at 15:14
  • \$\begingroup\$ @Malachi It is a lot of strings, some date time objects and one or two ints. It would really take a lot to translate. The thing is that I am making an application to manage Children with Special Needs, their associations and their biological family. What you pulled out there is "CPRKvinde" which means "Social Security Number Woman". \$\endgroup\$ – OmniOwl Nov 27 '13 at 15:14
3
\$\begingroup\$

I find whenever I feel the need to use a #region, there's something fishy going on with my code; I see the same in your SendPlejeFamilieObjekt method: all those WriteLine/Flush calls look very suspicious. Why do you flush at every single line written?

Have you considered overriding PlejeFamilie.ToString() to return the equivalent string? You could do _sWriter.WriteLine(pfamilie.ToString(); and have no code to modify in that method the day PlejeFamilie needs a new member.

[scrolls down]

The same applies to BiologiskFamilie; whenever something changes in these classes, you have multiple places that must change. I would refactor this code so as to minimize the amount of code that needs to be written (therefore possible omissions and bugs) when a change becomes required.

Why is everything static? This class has disposable private fields, I think it should be an instance, and should implement IDisposable so it could be used in a using block and then the client code doesn't need to "remember" to call EndConnection.

\$\endgroup\$
  • \$\begingroup\$ The method is in the DBM class (Database Manager). You send an object to the "Utility" class, it will call one of the DBM methods. To do this, I decided to make the class and it's methods Static since then I won't have to instantiate the DBM in my Utility class. I flush because if I don't the buffer might be filled up with more lines and I need to read the object attribute by attribute. \$\endgroup\$ – OmniOwl Nov 27 '13 at 14:41
  • \$\begingroup\$ @Vipar the buffer will auto flush itself when it gets full, don't worry about that \$\endgroup\$ – ratchet freak Nov 27 '13 at 14:48
  • \$\begingroup\$ @ratchetfreak The I might misunderstand how the buffer works. I thought it only sent what I wrote when it was flushed or full. \$\endgroup\$ – OmniOwl Nov 27 '13 at 14:50
  • \$\begingroup\$ @Vipar: It's OK for the buffer to have more than one line in it at a time. The TCP/IP stack will by default bunch the lines up anyway and send the info in bigger chunks to save bandwidth (see Nagle's algorithm), whether you flush or not. In fact, TCP on both ends does its best to maintain the illusion of a continuous stream of bytes; at that level, there's no such thing as a discrete packet/datagram/frame/whatever anyway. Just make sure each line has a CRLF or whatever at the end, and the other end will know they're separate. \$\endgroup\$ – cHao Nov 27 '13 at 17:58
2
\$\begingroup\$

you have the connection cleanup just at the bottom of the method, so when an early return sneaks in there or an exception occurs the connection won't be cleaned up

either wrap it in a try-finally:

public static void SendPlejeFamilieObjekt(PlejeFamilie pfamilie)
{

    try{
        InitConnection();

    }
    finally{
         EndConnection();
    }
 }

or create a IDisposable that does the cleanup in its Dispose and use the using statement

\$\endgroup\$

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.