Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

There are a number of suggestions I can think of. Hope these are helpful. Well done for asking for feedback.

Consider clarifying in your question exactly what kind of improvements you're asking for. Do you want the class to be "reliable", "maintainable", "optimised for performance", "written in as few lines as possible", etc. etc. otherwise you will get very general feedback.

Hard coded URL prevents reuse

###Hard coded URL prevents reuse### MoveMove the URL resource into at least a string constant or better still into your application configuration (perhaps a web.config file). You may want to consider passing in the URL as a parameter. This would make your class more reusable and maybe even more easy to test.

What does the class do?

###What does the class do?### TheThe fact that the class loads a dictionary of words from a URL is currently "hidden" away in your constructor. The name "MyDictionary" is not a meaningful class name as it doesn't describe what the class is for or what it does. Consider options like WordVector.

Low level detail not hidden away

###Low level detail not hidden away### I'mI'm confused about the way you're loading the file using 1024 byte increments. What goal are you hoping to accomplish by doing the load in this way? This is a low level detail which could be hidden away, perhaps even in a separate class - something like "LargeURLLoader" which the WordVector class could use to load its list. The advantage of pulling the load out into another class would allow you to load files in different ways, and also to focus on the details of the actual load, and would allow you to load such files from other classes instead of only within the dictionary.

Good luck!

There are a number of suggestions I can think of. Hope these are helpful. Well done for asking for feedback.

Consider clarifying in your question exactly what kind of improvements you're asking for. Do you want the class to be "reliable", "maintainable", "optimised for performance", "written in as few lines as possible", etc. etc. otherwise you will get very general feedback.

###Hard coded URL prevents reuse### Move the URL resource into at least a string constant or better still into your application configuration (perhaps a web.config file). You may want to consider passing in the URL as a parameter. This would make your class more reusable and maybe even more easy to test.

###What does the class do?### The fact that the class loads a dictionary of words from a URL is currently "hidden" away in your constructor. The name "MyDictionary" is not a meaningful class name as it doesn't describe what the class is for or what it does. Consider options like WordVector.

###Low level detail not hidden away### I'm confused about the way you're loading the file using 1024 byte increments. What goal are you hoping to accomplish by doing the load in this way? This is a low level detail which could be hidden away, perhaps even in a separate class - something like "LargeURLLoader" which the WordVector class could use to load its list. The advantage of pulling the load out into another class would allow you to load files in different ways, and also to focus on the details of the actual load, and would allow you to load such files from other classes instead of only within the dictionary.

Good luck!

There are a number of suggestions I can think of. Hope these are helpful. Well done for asking for feedback.

Consider clarifying in your question exactly what kind of improvements you're asking for. Do you want the class to be "reliable", "maintainable", "optimised for performance", "written in as few lines as possible", etc. etc. otherwise you will get very general feedback.

Hard coded URL prevents reuse

Move the URL resource into at least a string constant or better still into your application configuration (perhaps a web.config file). You may want to consider passing in the URL as a parameter. This would make your class more reusable and maybe even more easy to test.

What does the class do?

The fact that the class loads a dictionary of words from a URL is currently "hidden" away in your constructor. The name "MyDictionary" is not a meaningful class name as it doesn't describe what the class is for or what it does. Consider options like WordVector.

Low level detail not hidden away

I'm confused about the way you're loading the file using 1024 byte increments. What goal are you hoping to accomplish by doing the load in this way? This is a low level detail which could be hidden away, perhaps even in a separate class - something like "LargeURLLoader" which the WordVector class could use to load its list. The advantage of pulling the load out into another class would allow you to load files in different ways, and also to focus on the details of the actual load, and would allow you to load such files from other classes instead of only within the dictionary.

Good luck!

Source Link

There are a number of suggestions I can think of. Hope these are helpful. Well done for asking for feedback.

Consider clarifying in your question exactly what kind of improvements you're asking for. Do you want the class to be "reliable", "maintainable", "optimised for performance", "written in as few lines as possible", etc. etc. otherwise you will get very general feedback.

###Hard coded URL prevents reuse### Move the URL resource into at least a string constant or better still into your application configuration (perhaps a web.config file). You may want to consider passing in the URL as a parameter. This would make your class more reusable and maybe even more easy to test.

###What does the class do?### The fact that the class loads a dictionary of words from a URL is currently "hidden" away in your constructor. The name "MyDictionary" is not a meaningful class name as it doesn't describe what the class is for or what it does. Consider options like WordVector.

###Low level detail not hidden away### I'm confused about the way you're loading the file using 1024 byte increments. What goal are you hoping to accomplish by doing the load in this way? This is a low level detail which could be hidden away, perhaps even in a separate class - something like "LargeURLLoader" which the WordVector class could use to load its list. The advantage of pulling the load out into another class would allow you to load files in different ways, and also to focus on the details of the actual load, and would allow you to load such files from other classes instead of only within the dictionary.

Good luck!

lang-java

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