2
\$\begingroup\$

Have a look to the following example. Clearly it's very hard to understand the meaning of each parameter passed to the string.Format to be replaced to the numeric sequence of {0}, {1}, ...

I want to improve the software quality: Maintainability\Code readability http://en.wikipedia.org/wiki/Software_quality

The problem is also bound to the difficulty of associating numbers to the corrisponding values in the template. i.e. at a first glance what is the meaning of {8}?

               @"<?xml version='1.0'?>
                <RemoteXMLData>
                    <Username agent_diff_type=""{10}"" usertype=""{5}"" ticket_language=""{6}"" min_slip_stake=""{7}"" max_stake=""{8}"" location=""{9}"">{0}</Username>
                    <Password>{1}</Password>
                    <Reseller>{2}</Reseller>
                    <Function>AutoLogin</Function>
                    <AppKey>{3}</AppKey>
                    <Sub>{4}</Sub>
                </RemoteXMLData>";

What should I change?

string autoLoginRequest = String.Format(
AutoLoginRequestTemplate, 
username, 
password,
Config.Val1, 
Config.Val2,
requestSub, 
Language, 
minValue,
maxValue, 
agentDiffType);
\$\endgroup\$
  • 2
    \$\begingroup\$ You could simply wrap the string.Format in a method with a proper name and documentation that shows what the output will be like. The IDE will do the rest. \$\endgroup\$ – jessehouwing Jan 28 '14 at 14:58
  • 2
    \$\begingroup\$ As a partly flippant answer to "What should I change?", perhaps languages! Or at least approaches: consider using templating or XML-specific tools such as Razor, XDocument or XSLT. I would never recommend generating XML by methods on String. \$\endgroup\$ – Michael Urman Jan 28 '14 at 16:12
  • \$\begingroup\$ @MichaelUrman: Ok and what about a templating engine? if you have time give me a better explanation by Ansvering so I can decide if mark your answer as accepted. How would you use Razon in a c# application? \$\endgroup\$ – Revious Jan 28 '14 at 16:58
  • \$\begingroup\$ R# mitigates this problem by highlighting format item and argument pairs. \$\endgroup\$ – Brian Jan 28 '14 at 21:06
  • 1
    \$\begingroup\$ I haven't actually used Razor myself, but Phil Haack has. If you're creating XML, stick with the XML-specific tools like meda's XDocument example. \$\endgroup\$ – Michael Urman Jan 29 '14 at 4:03
5
\$\begingroup\$

Like @jesse said you can have a custom method, maybe pass a user object to that method. Something like this:

class User
{
    public int AutoLoginRequestTemplate { get; set; }
    public string username { get; set; }
    public string password { get; set; }
    public string Val1 { get; set; }
    public string Val2 { get; set; }
    public string requestSub { get; set; }
    public string Language { get; set; }
    public int minValue { get; set; }
    public int maxValue { get; set; }
    public int agentDiffType { get; set; }


    /// <summary>
    /// Formats the user info.
    /// </summary>
    /// <param name="user">The user object.</param>
    /// <returns>A formated string</returns>
    public string FormatUserInfo(User user)
    {

        return String.Format("{0}{1}{2}{3}{4}{5}{6}{7}{8}{9}",
                                user.AutoLoginRequestTemplate,
                                user.username,
                                user.password,
                                user.Val1,
                                user.Val2,
                                user.requestSub,
                                user.Language,
                                user.minValue,
                                user.maxValue,
                                user.agentDiffType);
    }
}

Your question changed, now I see that you are trying to create an xml string.

For the sake of readabilty, I wouldnt use string.Format(), instead I would just do simple concatenation:

"<Password>"+passowrd+"</Password>"

Now I am not sure about performance, but here we are talking about readability (you probably can spend your time being productive at something else rather than trying to decide which type of concatenation is more efficient/readable)

Now in terms of best practice, I wouldn't create xml manually (just like I wouldnt try to create a JSON or CSV parser) instead I would use .NET framework to do this, which would certainly make it more readble, something like this:

        XElement xml = new XElement("RemoteXMLData",
            new XElement("Username",
                new XAttribute("agent_diff_type", agentDiffType),
                new XAttribute("agentDiffType", agentDiffType),
                new XAttribute("usertype", userType),
                new XAttribute("ticket_language", Language),
                new XAttribute("min_slip_stake", minValue),
                new XAttribute("max_stake", maxValue),
                new XAttribute("location", location),
                username),
            new XElement("Password",
                password),
            new XElement("Reseller",
                reseller),
            new XElement("Function",
                function),
            new XElement("AppKey",
                appKey),
            new XElement("Sub",
                sub)
        );
\$\endgroup\$
  • \$\begingroup\$ I've added some other details. The thing which I don't like is to use the replacement of numbers with parameters. Since at a first glance I can't understand the meaning of {8} for example. In PHP the replacement is very more readable with a simple {$minValue} in the text that will be replaced by the variable minValue \$\endgroup\$ – Revious Jan 28 '14 at 16:04
  • \$\begingroup\$ @Sam String.Format is one of the basic structures of C#. Maybe you should go and learn the language before you start getting into more difficult problems. BTW, the {8} means you are expecting the 8th Parameter in the list after the string. In this case it would be user.maxValue \$\endgroup\$ – Jeff Vanzella Jan 28 '14 at 17:01
  • 1
    \$\begingroup\$ @Sam I updated my answer \$\endgroup\$ – meda Jan 28 '14 at 17:01
  • \$\begingroup\$ @JeffVanzella: yes, I know it very well, but it's not human readable at first glance. You have to count and it's subject to mistakes.. it's not a good software engineering tecnique I think.. \$\endgroup\$ – Revious Jan 28 '14 at 17:03
  • 1
    \$\begingroup\$ like @urman said in his comment above use the right tools. In my answer is used Xdocument which is perfect for this. String Builder or format are just bad for such task because one day it will break \$\endgroup\$ – meda Jan 28 '14 at 17:11

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.