2009-05-13 4 views
2

J'ai suivi quelques cours il y a longtemps et pour être honnête, je n'ai jamais vraiment compris le concept des cours. Je suis récemment "revenu sur le cheval" et j'ai essayé de trouver une application dans le monde réel pour créer une classe.S'il vous plaît critiquer ma classe

vous avez pu voir que je suis en train d'analyser un grand nombre de données d'arbre généalogique qui est dans un très ancien et le format suranné appelé GEDCOM

J'ai créé une classe Gedcom Reader pour lire dans le fichier, la traiter et le rendre disponible comme deux listes qui contiennent les données que j'ai trouvé nécessaire d'utiliser

Plus important pour moi est que j'ai créé un cours pour le faire, donc je voudrais vraiment que les experts me disent ce que j'ai fait juste et ce que j'aurais pu faire mieux (je ne dirai pas faux parce que la chose fonctionne et c'est assez bon pour moi)

Classe:

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

Mise en œuvre:

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

Je suis particulièrement intéressé si quelque chose pouvait être fait sur les structures, je ne sais pas si comment je les utiliser dans la mise en œuvre est le plus grand mais encore une fois cela fonctionne

Merci beaucoup

+0

Merci pour l'étiquette fixée. J'étais sûr que je l'avais raison Google est même revenu critiquer mon travail, mais je suppose que vous ne pouvez pas faire confiance à Google pour votre orthographe. – Crash893

+0

Les questions "Critique mon code" appartiennent à codereview.se –

Répondre

4

pensées rapides:

  • Les types imbriqués ne doivent pas être visibles.
  • ValueTypes (structs) doit être immuable.
  • Les champs (variables de classe) ne doivent pas être publics. Exposez-les via des propriétés à la place.
  • Vérifiez les arguments passés pour les valeurs non valides, comme null.
+0

... Expose-les via les propriétés ... Mieux encore, au lieu de récupérer les données de la classe, ajoutez des fonctions à la classe pour les manipuler plutôt qu'extérieurement. Dites, ne demandez pas. –

+0

@ Kenneth et Simon. Je ne suis pas familier avec les termes avez-vous un exemple? – Crash893

3

Je vous suggère de vérifier cet endroit: http://refactormycode.com/.

Pour certaines choses rapides, votre nom est la plus grande chose que je commencerais à changer. Inutile d'utiliser ALL-CAPS ou des termes abrégés. En outre, FxCop aidera avec beaucoup de changements suggérés. Par exemple, FindIndexinArray serait nommé FindIndexInArray.

EDIT:

Je ne sais pas si cela est un bug dans votre code ou par la conception, mais FindIndexinArray, vous ne romps pas de votre boucle une fois que vous trouverez un match. Voulez-vous le premier (break) ou le dernier (aucun break) match dans le tableau?

+0

oui encore sur le sujet, une personne peut avoir plus d'une femme ou plus d'un enfant donc je pourrais vouloir retourner les index de tous les amis ou tous les enfants en ce moment je fais juste le premier qu'il trouve – Crash893

+0

Je ne sais pas exactement ce que refactormycode.com est-ce que vous pourriez expliquer? – Crash893

+0

Eh bien, fondamentalement ce que vous demandez ici: vous code postal pour les gens pour aider refactor - c'est-à-dire, espérons améliorer. –

4

Il pourrait être plus lisible. C'est difficile à lire et à comprendre.

Vous pouvez étudier les principes solides (http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod)

Robert C.Martin a donné une bonne présentation sur Oredev 2008 sur le code propre (http://www.oredev.org/topmenu/video/agile/robertcmartincleancodeiiifunctions.4.5a2d30d411ee6ffd2888000779.html)

Quelques livres Recommandés à lire sur la lisibilité du code:

  • Kent Beck "modèles Implémentation"

  • Robert C Martin « code propre "Robert C

  • Martin "Principes Agile, modèles et pratiques en C#"

+1

+1 pour mentionner les principes SOLID. –

Questions connexes