1

Though it might be trivial for someone, I find it a little inconvenient when someone formats the string while passing it as a parameter to a method. For e.g.

AddMessage( string.Format("{0} (" + Constants.Message1 + ")",
    Path.GetFileNameWithoutExtension(document.FileName)),           
    string.Format("{0}" + FileExtensionPdf,
    Path.GetFileNameWithoutExtension(document.FileName)));

I have taken a simple example here, but it could become a little messy sometimes. I prefer to not do this and instead format my string before I pass them to the method. For me, this reduces readability.

I would like to know if there is a standard practice which goes against the above style. Since I am doing a code review, I am not sure if I should put it as a comment.

  • 2
    Never do or advocate something "because others do it". That is really all that "standard" or "practice" means. Do it because it provides a benefit over doing it differently. Formatting messages with substitutions like printf does greatly increases readability, so that is the reason you should be giving. – Kilian Foth Jan 12 '16 at 13:58
  • 1
    In the example code you are using both formatting and concatenation, which looks a bit strange. That's the first thing I would comment in a code review. – Torbjørn Jan 12 '16 at 14:10
  • 4
    C# 6 added string interpolation which greatly helps readability in string formatting: msdn.microsoft.com/en-us/library/dn961160.aspx – 17 of 26 Jan 12 '16 at 14:11
  • 1
    There will always be devs who sacrifice readability for brevity. Some (mainly from non-C/C++ backgrounds) despise the ternary operator for example. – Robbie Dee Jan 12 '16 at 14:12
  • @17of26 - The link says service is unavailable. – Shakti Prakash Singh Jan 12 '16 at 14:28
6

I would probably format your example code something like this:

var fileName = Path.GetFileNameWithoutExtension(document.FileName);
AddMessage(
    foo: string.Format("{0} ({1})", fileName, Constants.Message1),
    bar: string.Format("{0}{1}", fileName, FileExtensionPdf));

I think it's fine to format your input directly in the call as long as you do it in a readable way. Here I also use named arguments since it's two string arguments and I want to communicate clearly and avoid the stupid mistake of swapping those arguments.

Using a temporary varible for the file name with no extension is both an optimization of code and readability.

  • +1 Much better. The redundant code is removed, the author gets to keep their inline formatting and there are named arguments. – Robbie Dee Jan 12 '16 at 14:21
  • This looks much better as well as concise. You expanded it just enough make it cleaner and more readable. – Shakti Prakash Singh Jan 12 '16 at 14:31
4

I would simplify Torbjørn's answer with string interpolation:

var fileName = Path.GetFileNameWithoutExtension(document.FileName);
AddMessage($"{fileName} ({Constants.Message1})", $"{fileName}{FileExtensionPdf}");
  • 1
    I haven't started using C# 6 yet, but this is obviously very nice :) – Torbjørn Jan 12 '16 at 16:05
  • Me neither but certainly a bauble to look forward to... – Robbie Dee Jan 12 '16 at 16:20
  • it is neat, but i do feel that we lose something when we move away from the explicit use of the variables – Ewan Jan 12 '16 at 16:21
  • The explicit variables are still there! – 17 of 26 Jan 12 '16 at 16:41
1

I agree expanding it out makes it more readable and maybe easier to debug. not sure I would raise it as a problem though.

const string msgFormat1 = "{0} ({1})";
const string msgFormat2 = "{0}.{1}";

string fileName = Path.GetFileNameWithoutExtension(document.FileName);

string msg1 = String.Format(
    msgFormat1,
    Constants.Message1, 
    fileName
);

string msg2  = string.Format(
    msgFormat2, 
    fileName,
    FileExtensionPdf,
);

AddMessage(msg1, msg2);

edit : stole filename optimization from other answer

  • 2
    Creating msgFormat1 and msgFormat2 moves the formats further away from where they are used, making it harder to read now (in my opinion). – Torbjørn Jan 12 '16 at 14:20
  • I guess it depends somewhat on the rest of the code on whether one design is demonstrably better than another. ie are the formats used elsewhere/loaded in from config etc – Ewan Jan 12 '16 at 14:25
  • Yes, I agree. I will always inline the formats if I can though (if they are unique to this call to Format, if they are not dynamically loaded, and so on..) – Torbjørn Jan 12 '16 at 14:28
  • I agree with @Torbjørn - not sure the const statements add anything here, but still much better than the original... – Robbie Dee Jan 12 '16 at 14:38
  • @RobbieDee - I agree this is much better than the original one. But I think I would go with Torbjørn's solution. In fact, I probably would have thought of this if I would have done it myself, but when I look at it, the arguments of AddMessage call somewhat do not provide as to what is the information that is being passed. Though, it makes the code much cleaner. – Shakti Prakash Singh Jan 12 '16 at 14:43

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.