Skip to main content
added 26 characters in body
Source Link

Apart from @DJurcau's answer (with which I agree):

  1. You're not closing your StreamReaders.

  2. /// <returns>HTML Text of URL</returns>

Not always. It can also return an error.

  1. Exposing crucial request settings as public properties (rather than eg. method or constructor parameters) is questionable too, in my opinion. It doesn't enforce calling code to supply the necessary minimum of information required for the request to succeed. So it's not a particularly friendly API, if I the burden of remembering which properties need to be set is still on me.

Your SendRequest method doesn't even validate it explicitly - eg. if Url is not set, I'll just get an exception from WebRequest complaining about the lack of requestUriString, and you're leaving it up to me to figure out that it translates to the Url not being set. Same with RequestMethod.

Such mutability also introduces an inherent lack of thread safety - what if someone changes one of these properties (from another thread) while SendRequest is being executed and is half-way through? We don't always need thread safety, but it's one thing not to implement it at all, sort of ignoreignoring the issue altogether, and another: to throw it out of the window by design, for no good reason. Other things being equal, I'd say prefer stateless/immutable objects to stateful/mutable ones.

  1. Naming: if the class is named Request already, there's little point in naming all its properties RequestMethod, RequestParameters, RequestCookie etc. (with curious omission of Url - why not RequestUrl, then?). It's known as Smurf naming convention anti-pattern.

Apart from @DJurcau's answer (with which I agree):

  1. You're not closing your StreamReaders.

  2. /// <returns>HTML Text of URL</returns>

Not always. It can also return an error.

  1. Exposing crucial request settings as public properties (rather than eg. method or constructor parameters) is questionable too, in my opinion. It doesn't enforce calling code to supply the necessary minimum of information required for the request to succeed. So it's not a particularly friendly API, if I the burden of remembering which properties need to be set is still on me.

Your SendRequest method doesn't even validate it explicitly - eg. if Url is not set, I'll just get an exception from WebRequest complaining about the lack of requestUriString, and you're leaving it up to me to figure out that it translates to the Url not being set. Same with RequestMethod.

Such mutability also introduces an inherent lack of thread safety - what if someone changes one of these properties (from another thread) while SendRequest is being executed? We don't always need thread safety, but it's one thing not to implement it at all, sort of ignore the issue altogether, and another: to throw it out of the window by design, for no good reason.

  1. Naming: if the class is named Request already, there's little point in naming all its properties RequestMethod, RequestParameters, RequestCookie etc. (with curious omission of Url - why not RequestUrl, then?). It's known as Smurf naming convention anti-pattern.

Apart from @DJurcau's answer (with which I agree):

  1. You're not closing your StreamReaders.

  2. /// <returns>HTML Text of URL</returns>

Not always. It can also return an error.

  1. Exposing crucial request settings as public properties (rather than eg. method or constructor parameters) is questionable too, in my opinion. It doesn't enforce calling code to supply the necessary minimum of information required for the request to succeed. So it's not a particularly friendly API, if I the burden of remembering which properties need to be set is still on me.

Your SendRequest method doesn't even validate it explicitly - eg. if Url is not set, I'll just get an exception from WebRequest complaining about the lack of requestUriString, and you're leaving it up to me to figure out that it translates to the Url not being set. Same with RequestMethod.

Such mutability also introduces an inherent lack of thread safety - what if someone changes one of these properties (from another thread) while SendRequest is being executed and is half-way through? We don't always need thread safety, but it's one thing not to implement it at all, sort of ignoring the issue altogether, and another: to throw it out of the window by design, for no good reason. Other things being equal, I'd say prefer stateless/immutable objects to stateful/mutable ones.

  1. Naming: if the class is named Request already, there's little point in naming all its properties RequestMethod, RequestParameters, RequestCookie etc. (with curious omission of Url - why not RequestUrl, then?). It's known as Smurf naming convention anti-pattern.
added 375 characters in body
Source Link

Apart from @DJurcau's answer (with which I agree):

  1. You're not closing your StreamReaders.

  2. /// <returns>HTML Text of URL</returns>

Not always. It can also return an error.

  1. Exposing crucial request settings as public properties (rather than eg. method or constructor parameters) is questionable too, in my opinion. It doesn't enforce calling code to supply the necessary minimum of information required for the request to succeed. So it's not a particularly friendly API, if I the burden of remembering which properties need to be set is still on me.

Your SendRequest method doesn't even validate it explicitly - eg. if Url is not set, I'll just get an exception from WebRequest complaining about the lack of requestUriString, and you're leaving it up to me to figure out that it translates to the Url not being set. Same with RequestMethod.

Such mutability also introduces an inherent lack of thread safety - what if someone changes one of these properties (from another thread) while SendRequest is being executed? We don't always need thread safety, but it's one thing not to implement it at all, sort of ignore the issue altogether, and another: to throw it out of the window by design, for no good reason.

  1. Naming: if the class is named Request already, there's little point in naming all its properties RequestMethod, RequestParameters, RequestCookie etc. (with curious omission of Url - why not RequestUrl, then?). It's known as Smurf naming convention anti-pattern.

Apart from @DJurcau's answer (with which I agree):

  1. You're not closing your StreamReaders.

  2. /// <returns>HTML Text of URL</returns>

Not always. It can also return an error.

  1. Exposing crucial request settings as public properties (rather than eg. method or constructor parameters) is questionable too, in my opinion. It doesn't enforce calling code to supply the necessary minimum of information required for the request to succeed. So it's not a particularly friendly API, if I the burden of remembering which properties need to be set is still on me.

Your SendRequest method doesn't even validate it explicitly - eg. if Url is not set, I'll just get an exception from WebRequest complaining about the lack of requestUriString, and you're leaving it up to me to figure out that it translates to the Url not being set. Same with RequestMethod.

  1. Naming: if the class is named Request already, there's little point in naming all its properties RequestMethod, RequestParameters, RequestCookie etc. (with curious omission of Url - why not RequestUrl, then?). It's known as Smurf naming convention anti-pattern.

Apart from @DJurcau's answer (with which I agree):

  1. You're not closing your StreamReaders.

  2. /// <returns>HTML Text of URL</returns>

Not always. It can also return an error.

  1. Exposing crucial request settings as public properties (rather than eg. method or constructor parameters) is questionable too, in my opinion. It doesn't enforce calling code to supply the necessary minimum of information required for the request to succeed. So it's not a particularly friendly API, if I the burden of remembering which properties need to be set is still on me.

Your SendRequest method doesn't even validate it explicitly - eg. if Url is not set, I'll just get an exception from WebRequest complaining about the lack of requestUriString, and you're leaving it up to me to figure out that it translates to the Url not being set. Same with RequestMethod.

Such mutability also introduces an inherent lack of thread safety - what if someone changes one of these properties (from another thread) while SendRequest is being executed? We don't always need thread safety, but it's one thing not to implement it at all, sort of ignore the issue altogether, and another: to throw it out of the window by design, for no good reason.

  1. Naming: if the class is named Request already, there's little point in naming all its properties RequestMethod, RequestParameters, RequestCookie etc. (with curious omission of Url - why not RequestUrl, then?). It's known as Smurf naming convention anti-pattern.
Source Link

Apart from @DJurcau's answer (with which I agree):

  1. You're not closing your StreamReaders.

  2. /// <returns>HTML Text of URL</returns>

Not always. It can also return an error.

  1. Exposing crucial request settings as public properties (rather than eg. method or constructor parameters) is questionable too, in my opinion. It doesn't enforce calling code to supply the necessary minimum of information required for the request to succeed. So it's not a particularly friendly API, if I the burden of remembering which properties need to be set is still on me.

Your SendRequest method doesn't even validate it explicitly - eg. if Url is not set, I'll just get an exception from WebRequest complaining about the lack of requestUriString, and you're leaving it up to me to figure out that it translates to the Url not being set. Same with RequestMethod.

  1. Naming: if the class is named Request already, there's little point in naming all its properties RequestMethod, RequestParameters, RequestCookie etc. (with curious omission of Url - why not RequestUrl, then?). It's known as Smurf naming convention anti-pattern.