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

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

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

Eric Gunnerson C# Compiler Program Manager Microsoft Corporation

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;

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

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

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() {...}

Improper type names...

class DanceMove { point point offset1; 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() {...}

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() {...}

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_”)

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

Bad Code Classification Perplexing code Overly complex code Performance issue Bug farm

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

Robust by Default

switch (s) { case "a": HitA(); break; case "b": HitB(); break; }

Snippet #2

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

Bad rethrow

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

Good Rethrow

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

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

Bad because

User has to write this...

try { UpdateBalance(newBalance); } catch (Exception e) { if (e.InnerException != null && e.InnerException.GetType() == typeof(DatabaseConnectException)) { // retry with secondary database } }

Bad because

Easy to forget this...

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

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

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); } }

Provide an alternative

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

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 “look inside” nobody would ever want to Require an exception in a common path

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

Snippet #6

(); // clean up temporary objects }

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

Snippet #7

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

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?

Use “using” statement

{ string line; } } } { } } ProcessLine(line); ProcessLine(line); finally { if (reader != null) ((IDisposable) reader).Dispose(); } }

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

Nulling out locals

When it might help

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

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

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

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);

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

Unnecessary Cleanup

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

Snippet #11

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

Allocation in Loop

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

A better way

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

Snippet #12

enum ResponseType { ReturnValues, InvalidInput } string CreateWebText(string userInput, ResponseType operation) { switch (operation) { case ResponseType.ReturnValues: userInput = "

Values

" + FilterOutBadStuff(userInput); break; case ResponseType.InvalidInput: userInput = "
Invalid
" + FilterOutBadStuff(userInput); break; } return userInput; } s = CreateWebText(s, (ResponseType) 133); 10

Checking – Method #1

string CreateWebText(string userInput, ResponseType operation) { if (!Enum.IsDefined(typeof(ResponseType), operation) throw new InvalidArgumentException(...); } switch (operation) { case ResponseType.ReturnValues: userInput = "

Values

" + FilterOutBadStuff(userInput); break; } case ResponseType.InvalidInput: userInput = "
Invalid
" + FilterOutBadStuff(userInput); break; return userInput; enum ResponseType { ReturnValues, InvalidInput, DumpValues, }

Checking – preferred

string CreateWebText(string userInput, ResponseType operation) { switch (operation) { case ResponseType.ReturnValues: userInput = "

Values

" + FilterOutBadStuff(userInput); break; case ResponseType.InvalidInput: userInput = "
Invalid
" + FilterOutBadStuff(userInput); break; default: throw new InvalidArgumentException(...); } return userInput; }

Guidelines: enums Don’t

Assume that enums can only have defined values Assume that the set of defined values will never change

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

InvokeMember Slow

InvokeMember ( "Func”, BindingFlags.Public, null, pluginInstance, parameters); }

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); }

Snippet #14

foreach (Bignum b { { in c) } } 10

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() {...} } }

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() {...} } }

Guideline enumerable classes Do

Use the strongly-typed pattern-based approach Implement the interface-based version explicitly Using iterators and generic types instead for VS 2005 code

Snippet #15

public struct Rect { public int left; public int top; public int right; public int bottom; } // Wrap Win32 GetWindowRect Function // BOOL GetWindowRect(HWND hWnd, LPRECT lpRect); [DllImport("user32.dll")] private unsafe static extern bool GetWindowRect(IntPtr hwnd, Rect* rectangle); 10

Unnecessary Unsafe

Use “ref” instead

Snippet #16

class Notifier { ArrayList items = new ArrayList(); public void Add(object o) { lock (this) { items.Add(o); } } } 10

Lock on “this”

class Notifier { ArrayList items = new ArrayList(); public void Add(object o) { lock ( this ) { items.Add(o); } } } class UserClass { Notifier notifier; public void Add(object o) { lock (notifier) { notifier.Add(o); } } }

Lock on a private var

class Notifier { ArrayList items = new ArrayList(); public void Add(object o) { lock ( items ) { items.Add(o); } } }

Guideline - Locking Don’t

Use lock(this) Use lock(typeof())

Do

Lock on a private variable, not on something the user can see Use “object key = new object()” if you need a private key to lock on

Snippet #17

class ConsoleCtrl { public delegate void ControlHandler(ConsoleEvent consoleEvent); [DllImport("kernel32.dll")] static extern bool SetConsoleCtrlHandler( ControlEventHandler e, bool add); } public ConsoleCtrl() { SetConsoleCtrlHandler(new ControlHandler(Handler), true); } 10

Forgotten Delegate

Save in instance field

class ConsoleCtrl { public delegate void ControlHandler(ConsoleEvent consoleEvent); [DllImport("kernel32.dll")] static extern bool SetConsoleCtrlHandler( ControlEventHandler e, bool add); ControlHandler eventHandler; } public ConsoleCtrl() { // save this to a private var so the GC doesn't collect it...

eventHandler = new ControlHandler(Handler); SetConsoleCtrlHandler(eventHandler, true); }

Scoring Add up your score

16 possible points No fair using base 8 to increase your score No fair if you looked at the slides ahead of time

Anybody get them all?

You have to present next year...

Thank You

Resources CLR Profiler FXCop

C# Developer Center on MSDN http://msdn.microsoft.com/vcsharp C# Team Member Blogs http://msdn.microsoft.com/vcsharp/team/blogs/ Eric’s Blog http://blogs.msdn.com/ericgu

Please fill out a session evaluation on CommNet

Q1:

Overall satisfaction with the session

Q2: Q3: Q4:

Usefulness of the information Presenter’s knowledge of the subject Presenter’s presentation skills

Q5:

Effectiveness of the presentation

This presentation is for informational purposes only. Microsoft makes no warranties, express or implied, in this summary.