REFRESH!

[Minesweeper] 80 Games Played. 61 Bombs Used. 11771 Moves Performed. 8 New Users

[rubberduck-vba/Rubberduck] 1 issues closed. 8 issue comments

@SimonForsberg That's what I thought eheh

@Vogel612 Sounds like an attractive option. Would you want to do that on my existing map or would you want to start a new one?

@SimonForsberg your choice, but I'm more leaning towards doing it on the existing map, if only because you have a working mall :D

then again I would totally understand keeping your save to yourself

@Vogel612 I could always copy it

Let's talk more about it in October :)

@SimonForsberg yeah, that's a good idea. I should do that exam I planned for Oct. 7th...

hey

Would someone be able to review: github.com/vchittar/ChutesAndLadders

I just want to get an opinion on what people think and how my code looks like

i have about ~3.5 years of exp btw so be as harsh as kind as you want

@smarty_pants I would recommend grouping things by domain, not by type. It's unnecessary to put exceptions in their own package

I doubt you will need all those overloads for the CLException constructor

it's also unclear what CL stands for by just looking at a class

And often times other, already existing exceptions are good enough - have you checked what exceptions the Java library already contains?

package names should only contain lowercase letters

I hate Lombok. </rant>

Can GameBoard only support one chute and one ladder?

Player: initialRoll will probably always be zero?

@SimonForsberg yeah

thats why i initialized it as 0

A logger is not a constant and should therefore not be called LOG

@smarty_pants but it has a constructor which takes it as an argument

CL is just chutes and ladder exception

yeah i left it that way for testing, and wanted to leave it open in case someone had a different logic to play the game

PlayerUtils has a static variable which should probably not be static github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/…

also what do you mean grouping things by domain

ok thanks

@smarty_pants group things by what they are related to, not by the type they are. i.e. don't create a package for all exceptions, don't create a package for all services, etc. rather create a package for your application-specific things and one for the more generic Chutes and Ladders domain model

Hmm gotcha

i think i understand what youre saying and ill keep it in mind for next time

@SimonForsberg anything else?

still reading...

gotcha sorry haha

It's a bad practice to do have side-effects in lambdas (such as System.out.println within .stream().map )

Player position is an Integer but I don't think it can ever be null, use int instead

methods like this would make more sense to have within the Player class. github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/…

In fact, I think you should move a lot of your methods to the Player / GameBoard classes

Yeah that makes sense, I just thought it would be neater to have them as private classes down there

Your ladderMap and chuteMap are static, which they should not be. github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/… . And why are these not in in the GameBoard class?

but since its v specific to player state it should be moved over there

You're making a copy-paste error when you add your loggers to different classes, such as github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/… , and again: This is not a constant, it should be named log or logger, not LOG.

I originally had them as static because i thought i was going to reference them from the boardlogic or one of the main classes

@smarty_pants Even if you would do that, that's no excuse to have them static. Pass the data that your other classes need to your classes.

ok got it

what should the rule of thumb be to make something static or not

ladderMap and chuteMap could be just a single map. Source and destination. If destination > source, it's a ladder, if destination < source, it's a chute.

@smarty_pants Only make constants and loggers static (and final). That's it.

A logger is not a constant.

yeah that makes sense, i originally wanted to do that with the map but for some reason thought against it

cool thanks

Here you are throwing an exception, just to catch it, just to throw another exception: github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/…

Oh oops

how should that be handled

just remove the throws?

The use of the GameBoard data is confusing to me. github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/… Are you even using these values?

or just surround the whole thing within a try catch?

I am only using a couple of them to set the values so i can print the board

sorry print the progression of the players from one step to another

@smarty_pants I would probably remove this catch block github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/…

@smarty_pants I would recommend considering maybe make it a property on the player instead of the board. As multiple players can be on a chute/ladder at the same time(?)

or.... errr... the player is teleported directly to their destination... hrrm...

Yeah true, but at the end of the day all i need is the position before they got on the ladder or chute, position they were on, and the next position they got teleported to

so i thought having a gameboard would be a better bet to hold these values rather than putting this on the player

Organize your methods better... why is printBoard part of PlayerUtils? github.com/vchittar/ChutesAndLadders/blob/master/src/main/java/…

Because it prints the player status

and since it has to do with the player it goes in playerutils

but i think like you were saying earlier it could just go in the player pojo

@smarty_pants Then why is the method not called printPlayerStatus ?

Fair enough

@smarty_pants but for how long do you need the information about if a ladder/chute was used, and from where? Does it need to be persisted in the game state, or can it be passed between methods?

it need to from playerlogic to the print method

**print player status method

Thats why i thought if i used a pojo that just holds these values in the interim would be the best

@smarty_pants But I assume that right after it has happened, it will get printed, so as soon as the next move starts the data is not needed?

nope thats why that object is created once and just confined to the while loop till the game finishes

the prev turns data is never needed after the info gets printed out

I would recommend passing it around instead of persisting it. If nothing else, to get more practice in passing data around. And to reduce the confusion when looking at the GameBoard class (because it really did look like it was only supporting one chute and one ladder)

whats the difference

and where would i even initialize that to begin with?

@smarty_pants The difference is that you will be using memory on the stack instead of on the heap

the object just gets created once right? and the same objects data just gets overidden multiple times

hmm ok

but is it realistic to have a scenario where we would have a stack overflow error for this program

personally i dont see it, but i could be missing something

@smarty_pants When a move has been performed, then you can create an object with the information about what move was performed, and if it was a ladder/chute being used, from where and to where, etc. then you return that move object, and make sure that it is passed to the method printing the players.

@smarty_pants Only recursive calls I can see is when you are re-rolling dice for players, and that should not be deep enough to cause a stack overflow error

Ahh ok, so basically one pojo to essentially hold the player, prev move, next move and whether it was chute or a ladder

cool makes sense

@smarty_pants I would say "previous position and current position" rather than "prev move, next move"

right

Oh, and one more thing. Use Kotlin instead of Java 😁

haha

(Kotlin is a JVM-based language and is fully interoperable with Java)

I will once i get more and more comfortable with java. i am definitely getting there but i want to get really good at the language first from a oop design perspective

I feel like i am little bit behind the curve when it comes to someone with 3.5 years of xp so im just trying to close the gap.

thank you so much for doing this. i appreciate it.

what did you think about the reroll logic or the logic of the game in general? was it easy to follow and undertand?

You're welcome

@smarty_pants Worst part was that it was so spread out in multiple different classes. I would have preferred having mainly a Player class and a GameBoard class.

Yeah i figured i was doing too much when i was doing it

Thank you again!

would you say this is where you would expect someone with my years of exp would be or is there more work to be done?

@smarty_pants It's hard for me to say. Either way, you are where you are and I don't think it's worth thinking about where you should have been.

Damn that's something I need to tell myself that more often.

Haha ill take that as a little bit of a no

But makes sense, just to get better from here

@smarty_pants I have 21 years of experience now, it's hard for me to compare with how I was 18 years ago. 🤷‍♂️

im just razzin you a little bit haha

Good night :)