6
\$\begingroup\$

I'm trying to improve my functional skills and seeing where it fits and where it might not. Please review the code and see where functional practices might be applied. I'm specifically looking at trying to get rid of the state variables.

private int GetFirstLineFrameCount(XDocument doc)
{
    var subElement = (from res in doc.Descendants(_xmlns + "subelements")
                      where res.Element(_xmlns + "object")
                                 .Element(_xmlns + "metadata")
                                 .Element(_xmlns + "identifier").Value == "1"
                      select res).FirstOrDefault();

    var offsetElement = subElement.Descendants(_xmlnsEc + "start").Select(x => x.Element(_xmlnsEc + "editUnitNumber")).FirstOrDefault();

    if (offsetElement == null)
    {
        Logger.Error<SubtitleNormalizer>("no offset found");
        return 0;
    }

    return Int32.Parse(offsetElement.Value);
}
\$\endgroup\$
  • \$\begingroup\$ do you mean get rid of _xmlns and _xmlnsEc? If so just pass them in as parameters to the method. \$\endgroup\$ – dreza Aug 1 '13 at 19:32
1
\$\begingroup\$

A refactored functional programming that is evil.

public static class ObjectExtension
{
    public static Tout IfNotNull<Tin, Tout>(this Tin source, Func<Tin, Tout> func)
        where Tout : class
        where Tin : class
    {
        if (source == null)
        {
            return null;
        }
        else
        {
            return func(source);
        }
    }
}

public static class stringExtension
{
    public static int ToIntDefault(this string str, Func<int> def)
    {
        if (!string.IsNullOrEmpty(str))
        {
            return int.Parse(str);
        }
        return def();
    }
}

Consumer:

private int GetFirstLineFrameCount(XDocument doc, string _xmlns, string _xmlnsEc)
{
    int result = 0;
    var subElement = (from res in doc.Descendants(_xmlns + "subelements")
                      where res.Element(_xmlns + "object")
                                 .Element(_xmlns + "metadata")
                                 .Element(_xmlns + "identifier").Value == "1"
                      select res)
                      .FirstOrDefault()
                      .IfNotNull(k =>
                          k.Descendants(_xmlnsEc + "start").Select(x => x.Element(_xmlnsEc + "editUnitNumber")).FirstOrDefault()
                      )
                      .IfNotNull(k =>
                          {
                              result = k.Value.ToIntDefault(
                                  () => {
                                          Logger.Error<SubtitleNormalizer>("no offset found");
                                          return 0;
                                      }
                                  );
                              return (object)null;
                          }
                      );
    return result;
}

No, don't follow this. It is evil.

I just demonstrate something that can be done with functions. What you can do is as dreza's suggest to accept _xmlns and _xmlnsEc as parameter. Otherwise, I found that both of the variable is legal to exists, just make them readonly and do constructor injection. Otherwise, create a configuration for both variable so you can use myXmlConfiguration.Xmlns and myXmlConfiguration.XmlnsEc.

\$\endgroup\$
1
\$\begingroup\$

If you really want to make this functional, it looks like a case for the "Maybe monad". See

Unfortunately the dichotomy of class vs struct limits the elegance of Maybe monads in C#, but with the following:

public static class Maybe
{
    public static Maybe<TElement> ToMaybe<TElement>(this TElement value) where TElement : class
    {
        return new Maybe<TElement>(value);
    }
}

public struct Maybe<TElement> where TElement : class
{
    private readonly TElement _Wrapped;

    public bool HasValue { get { return _Wrapped != null; } }

    public TElement Value
    {
        get
        {
            if (_Wrapped == null) throw new InvalidOperationException();
            return _Wrapped;
        }
    }

    public Maybe(TElement value)
    {
        _Wrapped = value;
    }

    public TResult? SelectStruct<TResult>(Func<TElement, TResult> fn) where TResult : struct
    {
        return _Wrapped == null ? default(TResult?) : fn(_Wrapped);
    }

    // Returns "this" for fluent usage
    public Maybe<TElement> Exec(Action<TElement> ifAction, Action elseAction)
    {
        if (_Wrapped != null) ifAction(_Wrapped);
        else elseAction();
        return this;
    }
}

you can refactor

    var subElement = (from res in doc.Descendants(_xmlns + "subelements")
                      where res.Element(_xmlns + "object")
                                 .Element(_xmlns + "metadata")
                                 .Element(_xmlns + "identifier").Value == "1"
                      select res).FirstOrDefault();

    var offsetElement = subElement.Descendants(_xmlnsEc + "start").Select(x => x.Element(_xmlnsEc + "editUnitNumber")).FirstOrDefault();

    if (offsetElement == null)
    {
        Logger.Error<SubtitleNormalizer>("no offset found");
        return 0;
    }

    return Int32.Parse(offsetElement.Value);

to

    return (from res in doc.Descendants(_xmlns + "subelements")
            where res.Element(_xmlns + "object")
                     .Element(_xmlns + "metadata")
                     .Element(_xmlns + "identifier").Value == "1"
            select res).
           FirstOrDefault().
           Descendants(_xmlnsEc + "start").
           Select(x => x.Element(_xmlnsEc + "editUnitNumber")).
           FirstOrDefault().
           ToMaybe().
           Exec(val => {}, () => { Logger.Error<SubtitleNormalizer>("no offset found"); }).
           SelectStruct(elt => Int32.Parse(elt.Value)).
           GetValueOrDefault();

A fuller Maybe implementation might have a Where method, a Select<TResult>(Func<TElement, TResult>) where TResult : class method, and a GetValueOrDefault() method, and extension methods to do the same for Nullable<TStruct>, but I've simplified it to just support what's needed for this answer.

\$\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.