• Post Reply Bookmark Topic Watch Topic
  • New Topic

Need advice on my best piece of work  RSS feed

 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello,

I am not trying to show off but the following is my best piece of work I ever done, re-wrote this class like 20 times to make it this organized and as efficient as possible, I am sorry if it is long but whoever takes a look at it and gives me advice on how to make it better, more efficient and shorter, that would be lovely.

This class is responsible for registering weapons and their details in .dat files. Every weapon has a unique name and a code(class name) that no two weapons share. The class is also responsible to extract the data from the files when needed and display those on the interface that I am still to create. I have tried my best to maintain the methods names to be short and informative as possible and make them as easy as possible to read. Cheers and thanks in advance for any advice i receive.

 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I've already warned you about having lots of statics in your class before. Do you know about enum types? Your weapon types would be better defined as an enum type than as static fields in the Weapon class. Also, it's not a good idea to mix infrastructure concerns like code that deals with data file IO in a domain class like Weapon. That makes your design and code more brittle.
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Your Weapon constructor has way too many parameters. Limit to three or four at most. Any more than that, you should consider using a builder.
 
Campbell Ritchie
Marshal
Posts: 56529
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Formatting: Start by getting rid of the //===== comments. Separate logical parts of the program by single empty lines. Separate successive methods by single empty lines. Also break the long lines into several lines: read this. Also look at the old Sun style guide, which whou‍ld advise you how to shorten your lines.
Don't try to write a method all on one line. A method with a single statement shou‍ld occupy at least three lines. Don't write a loop or if‑else without {} and without indentation.

Why have you got public fields, particularly when some point to mutable reference types?
Why have you got those static methods?
There is something wrong about the addAttachs method. Apart from the fact that attachs is unpronounceable in English, that method appears to add the weapon to the attachment. That looks the wrong way round. Also, why are you rewriting the file so often?
Never write an empty catch block. If you have an exception in the deserialising method, why are you trying to catch it rather than letting it propagate?
 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:I've already warned you about having lots of statics in your class before.

Sorry, I thought You did not like a class containing statics only and I popped the question what do I do then with classes whose methods will only be used once but got no reply yet.

Junilu Lacar wrote:Do you know about enum types? Your weapon types would be better defined as an enum type than as static fields in the Weapon class.

I had no idea what enums were before you mentioned them now, I have googled it and looks neat, I think I will use it now to fix up a lot of code I got Cheers

Junilu Lacar wrote:Also, it's not a good idea to mix infrastructure concerns like code that deals with data file IO in a domain class like Weapon. That makes your design and code more brittle.

Problem is I have multiple types of Objects that are supposed to get stored in files and each one is stored in a completely different way, splitting them all up may take a lot of work in my opinion and if I am ever to come back to fix something or add something means I would have to look at multiple classes and get lost in them.

Junilu Lacar wrote:Any more than that, you should consider using a builder.
I have been reading on the builder since I read your post and will be fixing up all 7 classes I did similar to this one, thanks again Junili
 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:
Never write an empty catch block. If you have an exception in the deserialising method, why are you trying to catch it rather than letting it propagate?


Campbell Ritchie wrote:Formatting: Start by getting rid of the //===== comments. Separate logical parts of the program by single empty lines. Separate successive methods by single empty lines. Also break the long lines into several lines: read this. Also look at the old Sun style guide, which whou‍ld advise you how to shorten your lines.
Don't try to write a method all on one line. A method with a single statement shou‍ld occupy at least three lines. Don't write a loop or if‑else without {} and without indentation.

Yes Sir. Copy that will fix my code.

Campbell Ritchie wrote:Why have you got public fields, particularly when some point to mutable reference types?

Apart from the static ones, seems like all my field are private, or am I missing something?


Campbell Ritchie wrote:Why have you got those static methods?

Because I am dealing with other objects of this class, not the one created in this class, is there any other way to extract those objects out of the files without using static methods??

Campbell Ritchie wrote:There is something wrong about the addAttachs method. Apart from the fact that attachs is unpronounceable in English, that method appears to add the weapon to the attachment. That looks the wrong way round. Also, why are you rewriting the file so often?

Yeah, you're right, I have no idea why I did that will fix it or probably remove it until I remember, and next time I will place comments to remind myself why do I do stupid stuff like these. Every time I change something in the object I am required to re-write the file since there is no way I know of where I can directly edit a binary file.

Campbell Ritchie wrote:Never write an empty catch block. If you have an exception in the deserialising method, why are you trying to catch it rather than letting it propagate?
Again no idea why I did that, seems like something else I need to fix

Thanks, Campbell appreciate your time
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There is a design principle called the Single Responsibility Principle (SRP) that states that a class should have only one reason to change. By mixing in the concerns of file I/O and saving/reading instances from a file, you have increased the complexity of your Weapon class and given it multiple responsibilities. This makes your class more brittle and less cohesive. Concerns like file I/O and object persistence are best separated into a separate layer. Your concern about having to look at multiple classes and getting lost in them is largely unfounded if use packages properly. When you separate responsibilities out into different layers, e.g. presentation, domain, persistence, it's actually easier to maintain your application code. Small and cohesive classes are far easier to maintain than Big Balls of Mud, the term used to describe classes that multiple concerns jammed into one source file.
 
Knute Snortum
Sheriff
Posts: 4274
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yosuf Ibrahim wrote:
Junilu Lacar wrote:I've already warned you about having lots of statics in your class before.

Sorry, I thought You did not like a class containing statics only and I popped the question what do I do then with classes whose methods will only be used once but got no reply yet.

Only using a method once is no reason to make it static.
 
Knute Snortum
Sheriff
Posts: 4274
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yosuf Ibrahim wrote:
Campbell Ritchie wrote:Why have you got those static methods?

Because I am dealing with other objects of this class, not the one created in this class, is there any other way to extract those objects out of the files without using static methods??

You access a static method this way: where as you access a nonstatic method this way: The difference is that the static method belongs to the class, and it can only access static members.  The nonstatic method belongs to the instance you create and only that instance, and it can access any members in the class.

Maybe you can give us an illustration with code of what you're trying to accomplish with static members.
 
Knute Snortum
Sheriff
Posts: 4274
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You use this form you instantiate your Lists a lot: This is fine except for one thing: as much as possible, you should use the interface for the variable type.  In this case, that would look like this: Why would this be important?  Because you (or some developer after you) may want to use another implementation of List, for instance, for speed or other considerations.  It's a lot easier to change the implementation when the type is an interface.
 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Knute Snortum wrote:
Maybe you can give us an illustration with code of what you're trying to accomplish with static members.


I am trying to extract a list of objects of the type class from files stored in a directory. Therefore I used static methods to that. The methods are at the bottom half of my pasted code up top in the first post.

Cheers
 
Knute Snortum
Sheriff
Posts: 4274
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yosuf Ibrahim wrote:
Knute Snortum wrote:
Maybe you can give us an illustration with code of what you're trying to accomplish with static members.


I am trying to extract a list of objects of the type class from files stored in a directory. Therefore I used static methods to that. The methods are at the bottom half of my pasted code up top in the first post.

Again, you don't have to use static methods for that, although you may decide that you will.  Will only one list of these objects be needed throughout the whole application?  Then I would structure a utility class like this:
You would add your code for pulling the object list from the file to the method getObjectsFromFile(), and other classes would call SystemObjects.getObjects().  You would change the names of the members and class to something more descriptive.  This will work only if you need one copy of the object list for your entire app.
 
Campbell Ritchie
Marshal
Posts: 56529
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yosuf Ibrahim wrote:. . . Apart from the static ones, seems like all my field are private, or am I missing something? . . .
What makes you think that static fields, as shown in lines 30‑40, shou‍ld be public? Particularly since two fields point to mutable reference types.
Campbell Ritchie wrote:Why have you got those static methods?

Because I am dealing with other objects of this class, not the one created in this class, is there any other way to extract those objects out of the files without using static methods??
The deserialise method shou‍ld be static because it has to be called before any instances exist. But what about the others? Why are you using the existence of a file to show the existence of a weapon?
. . . Thanks, Campbell appreciate your time
That's a pleasure
 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:What makes you think that static fields, as shown in lines 30‑40, should be public? Particularly since two fields point to mutable reference types.

They should be public because this is not the only class I will be using them in, I will be using them in at least 3 other classes that are in my mind at this time, probably even more.

Campbell Ritchie wrote:Why are you using the existence of a file to show the existence of a weapon?

Every weapon is stored in it is own file and since the classnames is what makes them unique from each other I used that as the name of the .dat file that contains the weapon's details. The reason I am storing every weapon in it is own file is because it would be faster to find a weapon that way if it's class name is known saving a lot of good memory. I hope.

 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Knute Snortum wrote:...you don't have to use static methods for that...

You tell me I don't have to use static methods but proceed by telling me to create a class and in your example its all static methods only, I am a bit lost now.

 
Liutauras Vilda
Sheriff
Posts: 4917
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yosuf Ibrahim wrote:public static final String[] WEAPON_TYPES_LIST ...
public static final String[] WEAPON_TYPES_SEARCH_LIST ...

Don't call it a list, while it is an array. Anyway, this word LIST is redundant in any matter. I'd call it WEAPON_TYPES perhaps. And you might want to have them as enums rather than strings.

There is something fundamentally wrong with your code presentation. You continuously using comments to do painting. Comments need to be comments, in most cases JavaDoc comments, and occasionally extra explanatory comments, for instance - what 'dlc' means.

You also constantly omitting braces - that is not only dangerous, but makes code hardly readable. But if you still don't like them, are you familiar with Streams? Most of your methods look to me perfect candidates for consice substitutions with streams.

 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Liutauras Vilda wrote:
...are you familiar with Streams? Most of your methods look to me perfect candidates for consice substitutions with streams.

You are a wonder. Thank you. One quick read on streams and I already know how I can use it. Just gotta understand it properly so I can use it.

Cheers
 
Carey Brown
Saloon Keeper
Posts: 3310
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I was playing around with your software this afternoon and the first thing that becomes apparent (which I think has been mentioned in this thread) is that the persistence code needs to be pulled out and put in its own class (I called mine WeaponCollection). Once you do this the Weapon class looks more like a DAO (data access object) and is much, much, cleaner. Additionally, in my WeaponCollection class constructor I load all weapon files into a map and handle all adds/updates/deletes from there and only save it back out to files when close() is called.

I also looked into using Java8 predicates in WeaponCollection to perform searches so that the class itself doesn't need direct knowledge of how the search criteria is implemented. The class method boils down to this:

In the Weapon class I have this static method:
which generates a predicate that you can then pass to the WeaponCollection.search() method.

Interesting project you have here. Thanks for sharing it with us.
 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Carey Brown wrote:Interesting project you have here. Thanks for sharing it with us.


No man, thank you all, the amount of stuff I have learnt here is just amazing I am barely able to keep up. It is just amazing, last semester thanks to everyone here as well my java classes were useless, I turned out to be top of my class, handed in the best course project at the end of the semester all thanks to everyone here

I am busy at the moment and probably for the day re-writing my application piece by piece based on what you guys told me, I can't be thankful enough.

Cheers

 
Knute Snortum
Sheriff
Posts: 4274
127
Chrome Eclipse IDE Java Postgres Database VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yosuf Ibrahim wrote:
Knute Snortum wrote:...you don't have to use static methods for that...

You tell me I don't have to use static methods but proceed by telling me to create a class and in your example its all static methods only, I am a bit lost now.

Okay, I'll try to clarify.  You were saying that you had to use static methods to pull a list off the disk.  It's not required that you use static methods.  For instance, if each class needed to keep its own List of objects, you would use an instance method (nonstatic).  However, if the List of objects is global across the application -- that is, you only need one List everywhere -- then a utility class with static methods is the way to do it.
 
Yosuf Ibrahim
Ranch Hand
Posts: 128
4
Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I do not think databases would fit my needs because think of the objects I am saving as a words in a dictionary, user will be only reading that data will not modify it or add more to it, all the data will be added during the development of the application
 
Carey Brown
Saloon Keeper
Posts: 3310
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The data has to be put into persistent storage at some point so that the user can read it, even if that is by the developer. Database, flat file, CSV file, XML file, JSON file, properties file, serialized files, etc., are a variety of ways to persist storage of data. For the initial loading of data you might need a utility program to enter the data a persist it in you choice of approach, or a text editor if you choose a flat file.
 
Carey Brown
Saloon Keeper
Posts: 3310
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If your data is read-only, then why do you have
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!