Skip to content

Conversation

@bbSnavy
Copy link
Contributor

@bbSnavy bbSnavy commented May 8, 2023

No description provided.

Copy link
Owner

@macmv macmv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very neato, looking good for the most part.

Aside from the comments I left, you also need to run cargo fmt, and you should add trailing newlines to your .pand and .toml files.

Comment on lines +34 to +51
pub fn new(
ty: Type,
pos: FPos,
long_distance: bool,
offset: FPos,
count: u32,
data: f32,
) -> Self {
Self {
ty,
pos,
long_distance,
offset,
count,
data,
}
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the fields are public, no need for a constructor. Forcing users to use struct literals is also clearer, as you can see the field names.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the constructor is still here. forgot to push maybe?

@bbSnavy bbSnavy requested a review from macmv May 10, 2023 04:55
Copy link
Owner

@macmv macmv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking better, but it still needs some work. I think the reason I didn't add particles in the past is just because a 6-argument constructor is annoying. A builder would be much clearer, so I'd prefer to just have a builder and do it right the first time.

Comment on lines +34 to +51
pub fn new(
ty: Type,
pos: FPos,
long_distance: bool,
offset: FPos,
count: u32,
data: f32,
) -> Self {
Self {
ty,
pos,
long_distance,
offset,
count,
data,
}
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the constructor is still here. forgot to push maybe?

},
})
}
} No newline at end of file
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo fmt is the way. make sure you're on the nightly toolchain, and it should just fix all this.

Comment on lines +14 to +21
let particle = particle::Particle::new(
"witch",
player.pos(),
true,
util::FPos::new(0.0, 0.0, 0.0),
100,
0.0,
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike having a function take 6 arguments. I think using a builder like this would be clearer:

particle::Particle::builder("witch", player.pos())
  .long_distance(true)
  .velocity(0.0, 0.0, 0.0)
  .amount(100)
  .data(0.0)
  .build()

plus, the builder can have some defaults, so you don't need to set everything:

particle::Particle::builder("witch", player.pos())
  .long_distance(true)
  .amount(100)
  .build()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants