Peer Review of TCP Client Program Code

Question

I'd like to have this code looked over because I feel my approach might be very novice and there must have 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.

Thanks in advance!

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;
        }
    }
}

would you please translate your code and comments to English please. — Malachi, Nov 27 '13 at 14:55
@Malachi I have now changed the description of the class and put in comments in the code over the methods trying to translate what they mean. — Vipar, Nov 27 '13 at 15:02
are you worried about thread safety or not because currently it's not thread safe — ratchet freak, Nov 27 '13 at 15:07

ratchet freak · Answer 1 · 2013-11-27 14:55:29Z

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

Mat's Mug · Answer 2 · 2013-11-27 14:52:43Z

up vote 2 down vote

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 placed 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.

edited Nov 27 '13 at 14:52

answered Nov 27 '13 at 14:34

Mat's Mug♦
40.4k491281

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. – Vipar Nov 27 '13 at 14:41

@Vipar the buffer will auto flush itself when it gets full, don't worry about that – ratchet freak Nov 27 '13 at 14:48

@ratchetfreak The I might misunderstand how the buffer works. I thought it only sent what I wrote when it was flushed or full. – Vipar Nov 27 '13 at 14:50

@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. – cHao Nov 27 '13 at 17:58

add a comment |

asked	1 year ago
viewed	282 times
active	1 year ago

current community

your communities

more stack exchange communities

Peer Review of TCP Client Program Code

2 Answers 2

Your Answer

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

Visit Chat

Hot Network Questions

current community

your communities

more stack exchange communities

Peer Review of TCP Client Program Code

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

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

Visit Chat

Related

Hot Network Questions