Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Unified representation of ONE semantic

#Unified representation of ONE semantic IfIf "strRequestedReportID == null" and "".equals(strRequestedReportID) have the same semantic this should be unified as early as possible in previous code (maybe where the request occured) so the provided method and maybe all other methods need only to handle one case. I suggest to use an empty String that is the neutral element for String operations.

Responsibilities

#Responsibilities ThrowingThrowing an ExceptionInvalidReportRequest is not the responsibility of this method. Why ask for the usage of a cached ReportBean when it is not even passed in as a parameter? This should be handled before this method is called.

Multiple return statements

#Multiple return statements TryTry to avoid multiple returns. Breaking the control flow can hinder you to apply refactorings like extract method. So you should be sure that your code does not violate SRP or it will never change again when using multiple return statements, break or continue.

#Code in guessed environment

Code in guessed environment

As only one method for review is provided I can only guess how the environment looks like. So I made up a setup to include the method with the semantic I analyzed.

The point is: The method as provided is effectively one line. For me it makes perfect sense that "A cached report bean should only be used if it is the requested one". All other stuff I think does not belong to this method. Therefore it must be located in the environment I made up.

private BeanReport cachedReportBean;
private BeanReport getBeanReport(String strRequestedReportID) throws ExceptionInvalidReportRequest {
 
 // this should be done as early as possible 
 String normalizedRequestedReportID = strRequestedReportID == null ? "": strRequestedReportID;
 
 BeanReport reportBean = null;
 
 if ("".equals(normalizedRequestedReportID) && cachedReportBean == null) {
 throw new ExceptionInvalidReportRequest(
 "The request ID is invalid and no cached report exists.");
 }
 
 if (cachedReportBean != null && useCachedReportBean(cachedReportBean, normalizedRequestedReportID)) {
 ...
 reportBean = cachedReportBean;
 ...
 } else {
 ...
 reportBean = loadReportBean(normalizedRequestedReportID);
 ...
 }
 
 ...
 
 return reportBean;
}
private boolean useCachedReportBean(BeanReport cachedReportBean, String normalizedRequestedReportID) {
 return normalizedRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID())
}

#Unified representation of ONE semantic If "strRequestedReportID == null" and "".equals(strRequestedReportID) have the same semantic this should be unified as early as possible in previous code (maybe where the request occured) so the provided method and maybe all other methods need only to handle one case. I suggest to use an empty String that is the neutral element for String operations.

#Responsibilities Throwing an ExceptionInvalidReportRequest is not the responsibility of this method. Why ask for the usage of a cached ReportBean when it is not even passed in as a parameter? This should be handled before this method is called.

#Multiple return statements Try to avoid multiple returns. Breaking the control flow can hinder you to apply refactorings like extract method. So you should be sure that your code does not violate SRP or it will never change again when using multiple return statements, break or continue.

#Code in guessed environment

As only one method for review is provided I can only guess how the environment looks like. So I made up a setup to include the method with the semantic I analyzed.

The point is: The method as provided is effectively one line. For me it makes perfect sense that "A cached report bean should only be used if it is the requested one". All other stuff I think does not belong to this method. Therefore it must be located in the environment I made up.

private BeanReport cachedReportBean;
private BeanReport getBeanReport(String strRequestedReportID) throws ExceptionInvalidReportRequest {
 
 // this should be done as early as possible 
 String normalizedRequestedReportID = strRequestedReportID == null ? "": strRequestedReportID;
 
 BeanReport reportBean = null;
 
 if ("".equals(normalizedRequestedReportID) && cachedReportBean == null) {
 throw new ExceptionInvalidReportRequest(
 "The request ID is invalid and no cached report exists.");
 }
 
 if (cachedReportBean != null && useCachedReportBean(cachedReportBean, normalizedRequestedReportID)) {
 ...
 reportBean = cachedReportBean;
 ...
 } else {
 ...
 reportBean = loadReportBean(normalizedRequestedReportID);
 ...
 }
 
 ...
 
 return reportBean;
}
private boolean useCachedReportBean(BeanReport cachedReportBean, String normalizedRequestedReportID) {
 return normalizedRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID())
}

Unified representation of ONE semantic

If "strRequestedReportID == null" and "".equals(strRequestedReportID) have the same semantic this should be unified as early as possible in previous code (maybe where the request occured) so the provided method and maybe all other methods need only to handle one case. I suggest to use an empty String that is the neutral element for String operations.

Responsibilities

Throwing an ExceptionInvalidReportRequest is not the responsibility of this method. Why ask for the usage of a cached ReportBean when it is not even passed in as a parameter? This should be handled before this method is called.

Multiple return statements

Try to avoid multiple returns. Breaking the control flow can hinder you to apply refactorings like extract method. So you should be sure that your code does not violate SRP or it will never change again when using multiple return statements, break or continue.

Code in guessed environment

As only one method for review is provided I can only guess how the environment looks like. So I made up a setup to include the method with the semantic I analyzed.

The point is: The method as provided is effectively one line. For me it makes perfect sense that "A cached report bean should only be used if it is the requested one". All other stuff I think does not belong to this method. Therefore it must be located in the environment I made up.

private BeanReport cachedReportBean;
private BeanReport getBeanReport(String strRequestedReportID) throws ExceptionInvalidReportRequest {
 
 // this should be done as early as possible 
 String normalizedRequestedReportID = strRequestedReportID == null ? "": strRequestedReportID;
 
 BeanReport reportBean = null;
 
 if ("".equals(normalizedRequestedReportID) && cachedReportBean == null) {
 throw new ExceptionInvalidReportRequest(
 "The request ID is invalid and no cached report exists.");
 }
 
 if (cachedReportBean != null && useCachedReportBean(cachedReportBean, normalizedRequestedReportID)) {
 ...
 reportBean = cachedReportBean;
 ...
 } else {
 ...
 reportBean = loadReportBean(normalizedRequestedReportID);
 ...
 }
 
 ...
 
 return reportBean;
}
private boolean useCachedReportBean(BeanReport cachedReportBean, String normalizedRequestedReportID) {
 return normalizedRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID())
}
Source Link
oopexpert
  • 3.2k
  • 11
  • 17

#Unified representation of ONE semantic If "strRequestedReportID == null" and "".equals(strRequestedReportID) have the same semantic this should be unified as early as possible in previous code (maybe where the request occured) so the provided method and maybe all other methods need only to handle one case. I suggest to use an empty String that is the neutral element for String operations.

#Responsibilities Throwing an ExceptionInvalidReportRequest is not the responsibility of this method. Why ask for the usage of a cached ReportBean when it is not even passed in as a parameter? This should be handled before this method is called.

#Multiple return statements Try to avoid multiple returns. Breaking the control flow can hinder you to apply refactorings like extract method. So you should be sure that your code does not violate SRP or it will never change again when using multiple return statements, break or continue.

#Code in guessed environment

As only one method for review is provided I can only guess how the environment looks like. So I made up a setup to include the method with the semantic I analyzed.

The point is: The method as provided is effectively one line. For me it makes perfect sense that "A cached report bean should only be used if it is the requested one". All other stuff I think does not belong to this method. Therefore it must be located in the environment I made up.

private BeanReport cachedReportBean;
private BeanReport getBeanReport(String strRequestedReportID) throws ExceptionInvalidReportRequest {
 
 // this should be done as early as possible 
 String normalizedRequestedReportID = strRequestedReportID == null ? "": strRequestedReportID;
 
 BeanReport reportBean = null;
 
 if ("".equals(normalizedRequestedReportID) && cachedReportBean == null) {
 throw new ExceptionInvalidReportRequest(
 "The request ID is invalid and no cached report exists.");
 }
 
 if (cachedReportBean != null && useCachedReportBean(cachedReportBean, normalizedRequestedReportID)) {
 ...
 reportBean = cachedReportBean;
 ...
 } else {
 ...
 reportBean = loadReportBean(normalizedRequestedReportID);
 ...
 }
 
 ...
 
 return reportBean;
}
private boolean useCachedReportBean(BeanReport cachedReportBean, String normalizedRequestedReportID) {
 return normalizedRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID())
}
lang-java

AltStyle によって変換されたページ (->オリジナル) /