Loading ...
Sorry, an error occurred while loading the content.

Re: [altdotnet] Improving code quality

Expand Messages
  • Ayende Rahien
    ... Is my style, I find !isLastNamePart hard to read. This routine is part of parsing & processing a diff report. It accept a list of files & folders in
    Message 1 of 35 , Apr 10 9:01 PM
    View Source
    • 0 Attachment
      > if (isLastNamePart == false)

      Is my style, I find !isLastNamePart hard to read.

      This routine is part of parsing & processing a diff report. It accept a list of files & folders in particular versions, and need to spit out a list of changes.
      Because all I am getting is version & path, I need to parse that, taking into account folders, files and stuff.

      This piece is part of a large whole ( a method object with ~700 lines ).
      It doesn't make sense on its own, I am afraid.
      Which is why this is problematic to me.

      On Fri, Apr 11, 2008 at 6:50 AM, Bill Barry <after.fallout@...> wrote:
      I'll bite:

              if (isLastNamePart == false)
              {
                  folder = (FolderMetaData)item;
              }

      why isn't that "if(!isLastNamePart)"? Furthermore, if item is null, that assignment will happen twice wouldn't it?

      --

      if (item is StubFolderMetaData)
              {
      ...

               }
              else if (additionForPropertyChangeOnly.ContainsKey(item) && additionForPropertyChangeOnly[item])
              {
      ...
      these two ifs could be combined, because StubFolderMetaData is FolderMetaData

      --


          else if (item is DeleteFolderMetaData)
          {
              return;
          }

      this if can be brought up above the null check, in order to handle the simplest case first

      --

      Finally, does the null check mean that the item is missing? Either a comment or a method extraction would probably help here (but let's not start that flamewar again)? And what does isLastNamePart mean, is there a better name?


      Ayende Rahien wrote:
      As you probably know, I am currently working SvnBridge.
      The main challenge here is that SvnBridge contains a lot of code that is meant to bridge gaps between the conceptual model of Svn (Diff based) and TFS (Current state).
      This leads to a lot of complexity, as you can imagine. It is also a low level server, with fixed format and some crazy backend processing to get reasonable speeds out of it.

      The code is _well_ tested (close to a thousand unit tests, the last time I bothered to count), so modification is not a problem. The problem is where to take the code.
      The example below is a piece of code from the UpdateDiffCalculator, which takes a set of files and versions and turn that into a diff of file / folders to a known version.
      Just to make things interesting, SVN's working copy are not single version, in fact, each file can (and often does) have its own version, which needs to be resolved. And don't get me started on moving back in time.

      Anyway, this is to give you some example of what is required. I welcome all advice.
      The full source code can be had from: http://www.codeplex.com/SvnBridge/


      private void HandleDeleteItem(string remoteName, SourceItemChange change, string folderName, ref FolderMetaData folder, bool isLastNamePart, int targetVersion)
      {
          ItemMetaData item = folder.FindItem(folderName);
          if (item == null)
          {
              if (isLastNamePart)
              {
                  if (change.Item.ItemType == ItemType.File)
                  {
                      item = new DeleteMetaData();
                  }
                  else
                  {
                      item = new DeleteFolderMetaData();
                  }

                  item.Name = remoteName;
              }
              else
              {
                  item = sourceControlProvider.GetItemsWithoutProperties(targetVersion, folderName, Recursion.None);
                  if (item == null)
                  {
                      item = new DeleteFolderMetaData();
                      item.Name = folderName;
                  }
              }
              folder.Items.Add(item);
              if (isLastNamePart == false)
              {
                  folder = (FolderMetaData)item;
              }
          }
          else if (item is DeleteFolderMetaData)
          {
              return;
          }
          else if (isLastNamePart)// we need to revert the item addition
          {
              if (item is StubFolderMetaData)
              {
                  DeleteFolderMetaData removeFolder = new DeleteFolderMetaData();
                  removeFolder.Name = item.Name;
                  folder.Items.Remove(item);
                  folder.Items.Add(removeFolder);
              }
              else if (additionForPropertyChangeOnly.ContainsKey(item) && additionForPropertyChangeOnly[item])
              {
                  ItemMetaData removeFolder = item is FolderMetaData ? (ItemMetaData)new DeleteFolderMetaData() : new DeleteMetaData();
                  removeFolder.Name = item.Name;
                  folder.Items.Remove(item);
                  folder.Items.Add(removeFolder);
              }
              else
              {
                  folder.Items.Remove(item);
              }
          }
          if (isLastNamePart == false)
          {
              folder = (FolderMetaData)item;
          }
      }


    • Benny Å. Thomas
      The problem here is that the improvement is to isolated, if you look at the caller code, you can do even bigger improvements, I posted my solution. Benny
      Message 35 of 35 , Apr 14 6:05 AM
      View Source
      • 0 Attachment

        The problem here is that the improvement is to isolated, if you look at the caller code, you can do even bigger improvements, I posted my solution.

         

        Benny Thomas

         

        Fra: altdotnet@yahoogroups.com [mailto:altdotnet@yahoogroups.com] På vegne av Damien Guard
        Sendt: 14. april 2008 14:12
        Til: altdotnet@yahoogroups.com
        Emne: Re: [altdotnet] Re: Improving code quality

         

        I think Ayende was looking for code structure, refactoring, cyclic
        complexity rather than use of ! and placement of braces.

        I spent a few minutes refactoring it and came up with this:

        private void HandleDeleteItem(string remoteName, SourceItemChange
        change, string folderName, ref FolderMetaData folder, bool
        isLastNamePart, int targetVersion)
        {
        ItemMetaData item = folder.FindItem(folderName);
        if (item is DeleteFolderMetaData)
        return;

        if (item == null)
        item = ResolveDeleteFolderMetaData(remoteName, change, folderName,
        ref folder, isLastNamePart, targetVersion, ref item);
        else
        if (isLastNamePart)
        RevertItemAddition(folder, item);
        else
        folder = (FolderMetaData)item;
        }

        private static ItemMetaData ResolveDeleteFolderMetaData(string
        remoteName, SourceItemChange change, string folderName, ref
        FolderMetaData folder, bool isLastNamePart, int targetVersion)
        {
        ItemMetaData item = null;
        if (isLastNamePart)
        item = CreateMetaData(remoteName, change.Item.ItemType == ItemType.File);
        else {
        item = sourceControlProvider.GetItemsWithoutProperties(targetVersion,
        folderName, Recursion.None);
        if (item == null)
        item = new DeleteFolderMetaData(item.Name);
        }

        folder.Items.Add(item);
        if (!isLastNamePart)
        folder = (FolderMetaData)item;

        return item;
        }

        private static void RevertItemAddition(FolderMetaData folder, ItemMetaData item)
        {
        if (item is StubFolderMetaData)
        folder.Items.Add(new DeleteFolderMetaData(item.Name));
        else
        if (additionForPropertyChangeOnly.ContainsKey(item) &&
        additionForPropertyChangeOnly[item])
        folder.Items.Add(CreateMetaData(item.Name, item is FolderMetaData));

        folder.Items.Remove(item);
        }

        private static ItemMetaData CreateMetaData(string name, bool isFolder)
        {
        ItemMetaData item = (isFolder) ? new DeleteFolderMetaData() : new
        DeleteMetaData();
        item.Name = name;
        return item;
        }

        Still has too many smells for me, specifically could also benefit from:
        1. Getting rid of isLastName part and null checking - NullObjects?
        2. Passing ref around to either add to it or change it completely - very odd.
        3. Better handle the contains/null check in
        additionForPropertyChangeOnly with either a method to test both or
        prevent nulls getting in.

        [)amien

      Your message has been successfully submitted and would be delivered to recipients shortly.