views:

303

answers:

3

Hi all

I've taken a few school classes along time ago on and to be honest i never really understood the concept of classes. I recently "got back on the horse" and have been trying to find some real world application for creating a class.

you may have seen that I'm trying to parse a lot of family tree data that is in an very old and antiquated format called gedcom

I created a Gedcom Reader class to read in the file , process it and make it available as two lists that contain the data that i found necessary to use

More importantly to me is i created a class to do it so I would very much like to get the experts here to tell me what i did right and what i could have done better ( I wont say wrong because the thing works and that's good enough for me)

Class:

using System;
using System.Collections.Generic;
using System.Text;
using System.IO;

namespace GedcomReader
{

    class Gedcom
    {
        private string GedcomText = "";


        public struct INDI
        {
            public string ID;
            public string Name;
            public string Sex;
            public string BDay;
            public bool Dead;
        }
        public struct FAM
        {
            public string FamID;
            public string Type;
            public string IndiID;
        }

        public List<INDI> Individuals = new List<INDI>();
        public List<FAM> Families = new List<FAM>();


        public Gedcom(string fileName)
        {
            using (StreamReader SR = new StreamReader(fileName))
            {
                GedcomText = SR.ReadToEnd();
            }
            ReadGedcom();

        }

        private void ReadGedcom()
        {
            string[] Nodes = GedcomText.Replace("0 @", "\u0646").Split('\u0646');
            foreach (string Node in Nodes)
            {
                string[] SubNode = Node.Replace("\r\n", "\r").Split('\r');
                if (SubNode[0].Contains("INDI"))
                {
                    Individuals.Add(ExtractINDI(SubNode));

                }
                else if (SubNode[0].Contains("FAM"))
                {
                    Families.Add(ExtractFAM(SubNode));
                }
            }
        }

        private FAM ExtractFAM(string[] Node)
        {
            string sFID = Node[0].Replace("@ FAM", "");
            string sID = "";
            string sType = "";
            foreach (string Line in Node)
            {
                // If node is HUSB
                if (Line.Contains("1 HUSB "))
                {
                    sType = "PAR";
                    sID = Line.Replace("1 HUSB ", "").Replace("@", "").Trim();
                }
                //If node for Wife
                else if (Line.Contains("1 WIFE "))
                {
                    sType = "PAR";
                    sID = Line.Replace("1 WIFE ", "").Replace("@", "").Trim();
                }
                //if node for multi children
                else if (Line.Contains("1 CHIL "))
                {
                    sType = "CHIL";
                    sID = Line.Replace("1 CHIL ", "").Replace("@", "");
                }
            }
            FAM Fam = new FAM();
            Fam.FamID = sFID;
            Fam.Type = sType;
            Fam.IndiID = sID;

            return Fam;
        }
        private INDI ExtractINDI(string[] Node)
        {

            //If a individual is found
            INDI I = new INDI();
            if (Node[0].Contains("INDI"))
            {
                //Create new Structure

                //Add the ID number and remove extra formating
                I.ID = Node[0].Replace("@", "").Replace(" INDI", "").Trim();
                //Find the name remove extra formating for last name
                I.Name = Node[FindIndexinArray(Node, "NAME")].Replace("1 NAME", "").Replace("/", "").Trim();
                //Find Sex and remove extra formating
                I.Sex = Node[FindIndexinArray(Node, "SEX")].Replace("1 SEX ", "").Trim();

                //Deterine if there is a brithday -1 means no
                if (FindIndexinArray(Node, "1 BIRT ") != -1)
                {
                    // add birthday to Struct 
                    I.BDay = Node[FindIndexinArray(Node, "1 BIRT ") + 1].Replace("2 DATE ", "").Trim();
                }

                // deterimin if there is a death tag will return -1 if not found
                if (FindIndexinArray(Node, "1 DEAT ") != -1)
                {
                    //convert Y or N to true or false ( defaults to False so no need to change unless Y is found.
                    if (Node[FindIndexinArray(Node, "1 DEAT ")].Replace("1 DEAT ", "").Trim() == "Y")
                    {
                        //set death
                        I.Dead = true;
                    }
                }

            }
            return I;
        }
        private int FindIndexinArray(string[] Arr, string search)
        {
            int Val = -1;
            for (int i = 0; i < Arr.Length; i++)
            {
                if (Arr[i].Contains(search))
                {
                    Val = i;
                }
            }
            return Val;
        }
    }
}

Implementation:

using System;
using System.Windows.Forms;
using GedcomReader;

namespace WindowsFormsApplication1
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            string path = @"C:\mostrecent.ged";
            string outpath = @"C:\gedcom.txt";
            Gedcom GD = new Gedcom(path);

            GraphvizWriter GVW = new GraphvizWriter("Family Tree");
            foreach(Gedcom.INDI I in GD.Individuals)
            {
                string color = "pink";
                if (I.Sex == "M")
                {
                    color = "blue";
                }
                GVW.ListNode(I.ID, I.Name, "filled", color, "circle");

                if (I.ID == "ind23800")
                {MessageBox.Show("stop");}
                //"ind23800" [ label="Sarah Mandley",shape="circle",style="filled",color="pink" ];
            }
            foreach (Gedcom.FAM F in GD.Families)
            {

                if (F.Type == "par")
                {
                    GVW.ConnNode(F.FamID, F.IndiID);
                }
                else if (F.Type =="chil")
                {
                    GVW.ConnNode(F.IndiID, F.FamID);
                }
            }
           string x =  GVW.SB.ToString();
            GVW.SaveFile(outpath);
            MessageBox.Show("done");
        }
    }

I am particularly interested in if anything could be done about the structures i don't know if how i use them in the implementation is the greatest but again it works

Thanks alot

+3  A: 

Quick thoughts:

  • Nested types should not be visible.
  • ValueTypes (structs) should be immutable.
  • Fields (class variables) should not be public. Expose them via properties instead.
  • Check passed arguments for invalid values, like null.
Simon Svensson
...Expose them via properties... Better yet, instead of retrieving the data from the class, add functions to the class to manipulate it rather than externally. Tell, don't ask.
codeelegance
@ kenneth and Simon. im not to familar with the terms do you have an example?
Crash893
+2  A: 

I suggest you check this place out: http://refactormycode.com/.

For some quick things, your naming is the biggest thing I would start to change. No need to use ALL-CAPS or abbreviated terms.

Also, FxCop will help with a lot of suggested changes. For example, FindIndexinArray would be named FindIndexInArray.

EDIT:

I don't know if this is a bug in your code or by-design, but in FindIndexinArray, you don't break from your loop once you find a match. Do you want the first (break) or last (no break) match in the array?

Erich Mirabal
yea im still on the fense about that, a person may have more than one wife or more than one child so i might want to return the indexes of all the mates or all the children right now im just doing the first one it finds
Crash893
Im not exactly sure what refactormycode.com does could you explain
Crash893
Well, basically what you are asking here: you post code for people to help refactor - i.e. hopefully improve.
Erich Mirabal
I posted it on there. after looking threw some other code i don't feel to confident that it will indeed be refactored or refined at all that website while very neat does not seem quiet as active as ours ( that and im sure learning someone elses code for fun is not that appealing to most)
Crash893
+3  A: 

It might be more readable. It's hard to read and understand.

You may study SOLID principles (http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod)

Robert C. Martin gave good presentation on Oredev 2008 about clean code (http://www.oredev.org/topmenu/video/agile/robertcmartincleancodeiiifunctions.4.5a2d30d411ee6ffd2888000779.html)

Some recomended books to read about code readability:

  • Kent Beck "Implemetation patterns"

  • Robert C Martin "Clean Code" Robert C

  • Martin "Agile Principles, Patterns and Practices in C#"

Marek Tihkan
+1 for mentioning SOLID principles.
codeelegance