DEV320 Visual C# Best Practices: What’s Wrong With This Code?

1. DEV320 Visual C# Best Practices:What’s Wrong With This Code? Eric Gunnerson C# Compiler Program Manager Microsoft Corporation

2. Agenda eric.Introductions(); foreach (CodeExample e in Dev320.CodeExamples) { presentation.Display(e); Thread.Wait(10000); attendee.SuggestAnswer(); eric.Discuss(e); attendee.RecordScore(attendee.Answer == eric.Answer(e)); } score.totalUp(); goto AskTheExperts;

3. What is “bad code”? • Things to look for: • Possible bugs • Performance issues • Best practice violations • Things to ignore • Whether the code actually compiles • Missing code • Formatting • Things I didn’t think of

4. Practice class dance_move { point offset1; point offset2; float rotation1; float rotation2; public dance_move(int x1, int y1, int x2, int y2, float rotation1, float rotation2) { ... } public point GetOffset(float factor) { ... } public float GetRotation(float factor) {...} public void GetFunky() {...} } 10

5. Improper type names... class dance_move { point offset1; point offset2; float rotation1; float rotation2; public dance_move(int x1, int y1, int x2, int y2, float rotation1, float rotation2) { ... } public point GetOffset(float factor) { ... } public float GetRotation(float factor) {...} public void GetDown() {...} public void GetFunky() {...} }

6. Improper type names... class DanceMove { point offset1; point offset2; float rotation1; float rotation2; public DanceMove(int x1, int y1, int x2, int y2, float rotation1, float rotation2) { ... } public point GetOffset(float factor) { ... } public float GetRotation(float factor) {...} public void GetFunky() {...} }

7. Improper type names... class DanceMove { Point offset1; Point offset2; float rotation1; float rotation2; public DanceMove(int x1, int y1, int x2, int y2, float rotation1, float rotation2) { ... } public Point GetOffset(float factor) { ... } public float GetRotation(float factor) {...} public void GetDown() {...} public void GetFunky() {...} }

8. Guidance: Naming • Do • Use a consistent naming pattern • Use the .NET pattern for externally-visible items • Consider • Whether you want to use a field prefix (“_”, or “m_”)

9. Scorekeeping • Keep track of your score • 1 point per correct answer • 1 bonus point if you give the correct answer • On the honor system because • I trust you • The points don’t matter

10. Bad Code Classification • Perplexing code • Overly complex code • Performance issue • Bug farm

11. Snippet #1 switch (s) { case "a": try { HitA(); } catch (Exception e) { throw e; } break; case "b": try { HitB(); } catch (Exception e) { throw e; } break; } 10

12. Robust by Default switch (s) { case "a": HitA(); break; case "b": HitB(); break; }

13. Snippet #2 public void Process(string filename) { try { processor.Process(filename); } catch (Exception e) { writer.WriteLine(e); throw e; } } 10

14. Bad rethrow public void Process(string filename) { try { processor.Process(filename); } catch (Exception e) { writer.WriteLine(e); throw e; } }

15. Good Rethrow public void Process(string filename) { try { processor.Process(filename); } catch (Exception e) { writer.WriteLine(e); throw; } }

16. Snippet #3 void UpdateBalance() { try { balance = newBalance; db.UpdateBalance(accountID, newBalance); } catch (DatabaseException e) { throw new Exception( String.Format("Error updating balance on account {0} to {1}", accountID, newBalance), e); } } 10

17. Bad becauseUser has to write this... try { UpdateBalance(newBalance); } catch (Exception e) { if (e.InnerException != null && e.InnerException.GetType() == typeof(DatabaseConnectException)) { // retry with secondary database } }

18. Bad becauseEasy to forget this... try { UpdateBalance(newBalance); } catch (Exception e) { if (e.InnerException != null && e.InnerException.GetType() == typeof(DatabaseConnectException)) { // retry with secondary database } throw; }

19. Snippet #4 public struct Bignum { public static Bignum Parse(string number) { bool badNumber = false; // conversion code here... if (badNumber) { throw new ArgumentException("Bad string", “number”); } return new Bignum(number); } } 10

20. Exception Required public struct Bignum { public static Bignum Parse(string num) { bool badNumber = false; // conversion code here... if (badNumber) { throw new ArgumentException("Bad string", "num"); } return new Bignum(num); } }

21. Provide an alternative public struct Bignum { public static Bignum Parse(string num) {...} public static bool TryParse(string num, out Bignum result) {...} }

22. Guidelines: Exceptions • Don’t • Write unnecessary try-catch code • Behavior is correct by default • Rethrow using “throw e” • Loses stack frame information • Wrap in larger exception • Unless you’re sure nobody would ever want to “look inside” • Require an exception in a common path

23. Snippet #6 public void TransmitResponse(ArrayList responses, StreamWriter streamWriter) { foreach (DataResponse response in responses) { NetworkResponse networkResponse = response.GetNetwork(); networkResponse.Send(streamWriter); } GC.Collect(); // clean up temporary objects } 10

24. Snippet #6 public void TransmitResponse(ArrayList responses, StreamWriter streamWriter) { foreach (DataResponse response in responses) { NetworkResponse networkResponse = response.GetNetwork(); networkResponse.Send(streamWriter); } GC.Collect(); // clean up temporary objects }

25. Calling GC.Collect • Bad because... • Each collection has overhead • Overhead not really related to # of “dead objects” • Collecting one object is as expensive (roughly) as collecting one thousand • Especially bad because... • GC.Collect() does the most expensive collection

26. Snippet #7 void ReadFromFile(string filename) { StreamReader reader = File.OpenText(filename); string line; while ((line = reader.ReadLine()) != null) { ProcessLine(line); } reader.Close(); } 10

27. Latent Bug void ReadFromFile(string filename) { StreamReader reader = File.OpenText(filename); string line; while ((line = reader.ReadLine()) != null) { ProcessLine(line); } reader.Close(); } What if this line throws an exception?

28. Use “using” statement void ReadFromFile(string filename) { StreamReader reader = File.OpenText(filename);; try { string line; while ((line = reader.ReadLine()) != null) { ProcessLine(line); } } finally { if (reader != null) ((IDisposable) reader).Dispose(); } } void ReadFromFile(string filename) { using(StreamReader reader = File.OpenText(filename)) { string line; while ((line = reader.ReadLine()) != null) { ProcessLine(line); } } }

29. Snippet #8 public string GetCommaSeparated(Employee[] employees) { StringBuilder s = new StringBuilder(); for (int i = 0; i < employees.Length - 1; i++) { s.Append(employees[i].ToString() + ","); employees[i] = null; } s.Append(employees[employees.Length - 1].ToString()); return s.ToString(); } 10

30. Nulling out locals public string GetCommaSeparated(Employee[] employees) { StringBuilder s = new StringBuilder(); for (int i = 0; i < employees.Length - 1; i++) { s.Append(employees[i].ToString() + ","); employees[i] = null; } s.Append(employees[employees.Length - 1].ToString()); return s.ToString(); }

31. When it might help public static void SubmitTaxRecords(Employee[] employees) { for (int i = 0; i < employees.Length; i++) { SendGovernmentalData(employees[i]); employees[i] = null; } }

32. Guidance: Memory Management • Don’t • Call GC.Collect() • Null out locals • Unless a GC is likely to happen in a routine • Do • Let the GC do its work • Use the “using” statement or call Dispose() to help the GC clean up early

33. Snippet #9 public class Resource: IDisposable { IntPtr myResource; ~Resource() { Dispose(false); } public void Dispose() { Dispose(true); } protected virtual void Dispose(bool disposing) { FreeThatResource(myResource); } } 10

34. Missing SuppressFinalize public class Resource: IDisposable { IntPtr myResource ~Resource() { Dispose(false); } public void Dispose() { Dispose(true); } protected virtual void Dispose(bool disposing) { if (disposing) { GC.SuppressFinalize(this); } FreeThatResource(myResource); } }

35. Snippet #10 class Employee: IDisposable { public Employee(int employeeID, string name, string address) {...} ~Employee() { Dispose(false); } public void Dispose() { Dispose(true); } protected virtual void Dispose(bool disposing) { name = null; address = null; if (!disposing) { GC.SuppressFinalize(this); } } } 10

36. Unnecessary Cleanup class Employee: IDisposable { public Employee(int employeeID, string name, string address) {...} ~Employee() { Dispose(false); } public void Dispose() { Dispose(true); } protected virtual void Dispose(bool disposing) { name = null; address = null; if (!disposing) { GC.SuppressFinalize(this); } } }

37. Guidance:Finalization and IDisposable • Do • Implement a destructor and IDisposable if your object holds onto an unmanaged resource • Consider • What your usage pattern is before implementing IDisposable on a object that contains disposable objects. • Using GC.AddMemoryPressure() in .NET Framework V2.0

38. Snippet #11 public override string ToString() { string fullString = ""; foreach (string s in strings) { fullString += s + " : "; } return fullString; } 10

39. Allocation in Loop public override string ToString() { string fullString = ""; foreach (string s in strings) { fullString += s + " : "; } return fullString; }

40. A better way public string ToString() { StringBuilder fullString = new StringBuilder(); foreach (string s in strings) { fullString.Append(s); fullString.Append(“:”); } return fullString.ToString(); }

41. Snippet #12 enum ResponseType { ReturnValues, InvalidInput } string CreateWebText(string userInput, ResponseType operation) { switch (operation) { case ResponseType.ReturnValues: userInput = "<h1>Values</h1>" + FilterOutBadStuff(userInput); break; case ResponseType.InvalidInput: userInput = "<h1>Invalid</h1>" + FilterOutBadStuff(userInput); break; } return userInput; } s = CreateWebText(s, (ResponseType) 133); 10

42. Checking – Method #1 string CreateWebText(string userInput, ResponseType operation) { if (!Enum.IsDefined(typeof(ResponseType), operation) throw new InvalidArgumentException(...); switch (operation) { case ResponseType.ReturnValues: userInput = "<h1>Values</h1>" + FilterOutBadStuff(userInput); break; case ResponseType.InvalidInput: userInput = "<h1>Invalid</h1>" + FilterOutBadStuff(userInput); break; } return userInput; } enum ResponseType { ReturnValues, InvalidInput, DumpValues, }

43. Checking – preferred string CreateWebText(string userInput, ResponseType operation) { switch (operation) { case ResponseType.ReturnValues: userInput = "<h1>Values</h1>" + FilterOutBadStuff(userInput); break; case ResponseType.InvalidInput: userInput = "<h1>Invalid</h1>" + FilterOutBadStuff(userInput); break; default: throw new InvalidArgumentException(...); } return userInput; }

44. Guidelines: enums • Don’t • Assume that enums can only have defined values • Assume that the set of defined values will never change

45. Snippet #13 class Plugin { Type pluginType; object pluginInstance; public Plugin(Assembly a, string typeName) { pluginType = a.GetType(typeName); pluginInstance = Activator.CreateInstance(pluginType); } public double Calculate(params object[] parameters) { return (double) pluginType.InvokeMember( "Func”, BindingFlags.Public, null, pluginInstance, parameters); } } 10

46. InvokeMember Slow class Plugin { Type pluginType; object pluginInstance; public Plugin(Assembly a, string typeName) { pluginType = a.GetType(typeName); pluginInstance = Activator.CreateInstance(pluginType); } public double Calculate(params object[] parameters) { return (double) pluginType.InvokeMember( "Func”, BindingFlags.Public, null, pluginInstance, parameters); } }

47. Interface based approach public interface IPlugin { double Func(params double[] parameters); } class Plugin { IPlugin plugin; public Plugin(Assembly a, string typeName) { Type pluginType = a.GetType(typeName); plugin = (IPlugin) Activator.CreateInstance(pluginType); } public double Calculate(params double[] parameters) { return plugin.Func(parameters); } }

48. BignumCollection c = ...; foreach (Bignum b in c) { ... } BignumCollection c = ...; foreach (string s in c) { ... } Each value is boxed, then unboxed Not typesafe at compile time Snippet #14 public class BignumCollection: IEnumerable { public IEnumerator GetEnumerator() {...} struct BignumCollectionEnumerator: IEnumerator { public BignumCollectionEnumerator( BignumCollection collection) {...} public bool MoveNext() {...} public object Current { get {...} } public void Reset() {...} } } public class BignumCollection: IEnumerable { public IEnumerator GetEnumerator() {...} struct BignumCollectionEnumerator: IEnumerator { public BignumCollectionEnumerator( BignumCollection collection) {...} public bool MoveNext() {...} public object Current { get {...} } public void Reset() {...} } } 10

49. With pattern public class BignumCollection: IEnumerable { public BignumCollectionEnumerator GetEnumerator() {...} struct BignumCollectionEnumerator:IEnumerator { public BignumCollectionEnumerator( BignumCollection collection) {...} public bool MoveNext() {...} public Bignum Current { get {...} } public void Reset() {...} } }

50. With pattern and interface public class BignumCollection: IEnumerable { IEnumerator IEnumerable.GetEnumerator() { return GetEnumerator(); } public BignumCollectionEnumerator GetEnumerator() {...} struct BignumCollectionEnumerator: IEnumerator { public BignumCollectionEnumerator( BignumCollection collection) {...} public bool MoveNext() {...} object IEnumerator.Current { get { return Current; } } public Bignum Current { get {...} } public void Reset() {...} } }