This forum uses cookies
This forum makes use of cookies to store your login information if you are registered, and your last visit if you are not. Cookies are small text documents stored on your computer; the cookies set by this forum can only be used on this website and pose no security risk. Cookies on this forum also track the specific topics you have read and when you last read them. Please confirm whether you accept or reject these cookies being set.

A cookie will be stored in your browser regardless of choice to prevent you being asked this question again. You will be able to change your cookie settings at any time using the link in the footer.

Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Code review - Time in Service and Refurbishment
#1
Both are done and up to date (with 0.3.23) and in need of a good once over by any dev with a little free time. Does any feature need anything? Spot any bugs? Can something be done more efficiently?

If there is nothing else that needs to be changed I will put up a pull request.
Reply
#2
It would be better if you had them up to date with trunk so we could do a proper review. And please make sure these are separate pull requests when you make those.
Reply
#3
Should the procedure be to just make two pull requests?  I guess, I thought the purpose of the pull request was to do code review and see if it should get merged in.  There can be discussion over pull requests and updates made before it gets merged.
Reply
#4
Updated both. No conflicts for the moment.

As for this code "preview" isn't HQ, MM, and MML currently in a feature freeze until stable? If thats not the case I will go ahead and create pull requests now instead of waiting.

For now I had only planned on pull requests for my Unit changes so let me know if I should change that.
Reply
#5
Pull requests don't have to be fulfilled right away...
Reply
#6
Not to mention, time in service should be a small enough thing that we could potentially break freeze for it should the code meet our standards of quality. Refurbishment on the other hand is dangerous territory and would not.
Reply
#7
Another reason for the early review before I do a pull request for refurbishment. The code changes are as non intrusive as possible. There are no changes to the way refits work just additional code to handle refurbishments.

I guess I can go ahead and do a pull request for both if it will make things easier.

Additional question though. For other optionized content (such as the 3 other log saver ideas on my tracker) how should I handle them. As individual commits to the same branch or three separate branches?
Reply
#8
Typically with git one would work on each feature in it's own branch. Then, when it's ready, you merge that branch back into trunk. However, we don't really do that with MM for the team. However, on your fork you could if you chose. Then, once you perform the merge to the trunk, that merge commit becomes your pull request back to the repo you forked from and you close off the branch.
Reply
#9
They way we do it at work is that each contributor has their own fork. You open an issue on the main repo and then create a branch on your own fork. When the work is done you open a merge request and reference the issue. I personally think the issue think is sort of redundant but it kind of gives a heads up on who is working on what.

However, in another group, they don't fork. Instead they just open branches on the main repo, but the rest of the process is the same. The only problem is that only a few people have the permissions to review merge requests and put them into the main repo branch.

The think my work needs to learn from MegaMek is how to do a stable release and a dev release. It seems like at work I am always using the latest dev, which is difficult when you have jobs running for a week on the cluster and someone who is much higher than me decides it is time to update a submodule. 4 days gone! Lame!

SO,
I have no idea what process we want or if it even matters. Do you think and let's see what ways we like.
Reply
#10
I have also worked in a model where every issue becomes its own branch, where work is done on that branch and then merged back into master when the issue is resolved.  I have considered doing that for Megamek (and any individual dev could choose to do that).  I have done it for some things as well, but in general, many of the bug fixes end up being minor and I feel like it's additional work without a lot of benefit.
Reply
#11
Yeah, each issue is a branch is how my work does it as well.  Then each merge request has to be peer reviewed and at least one of the reviewers has to be a lead on the project.  It's definitely overkill when all you're doing is adding a simple null check but it does have value for large changes.
Reply
#12
(01-29-2016, 03:01 PM)Netzilla link Wrote: Yeah, each issue is a branch is how my work does it as well.  Then each merge request has to be peer reviewed and at least one of the reviewers has to be a lead on the project.  It's definitely overkill when all you're doing is adding a simple null check but it does have value for large changes.

It may be overkill, but it definitely helps enforce standards. Standards at my job include null first (see below), but some people (myself included) have some very bad habits where the null is after the variable Big Grin

One thing we should do is start writing up our development standards for the project on the GitHub wiki. Right now we don't have much other than indentation is 4 spaces, no tabs. I would like to see that change to include:

Null checks are always null first:
Code:
if (null == variable && other conditions) // null check is the first check, and the null is on the left side

Block Brace Style, Newline with indent:
Code:
if (some condition is true) // command that starts the block
{ // opening curly brace is on it's own line, but indented the same depth as the command that starts the block
    // code goes here, indented our already established 4 spaces!
} // closing curly brace is returned 4 spaces back to match with the opener
Reply
#13
Tongue I ask for a code review and instead I get reviews of the thread and coding standards review.

As for block brace... meh.. seldom have I seen it used out of all the projects I have helped out. As for the null checks, up until working on the MM/HQ code I have never seen it done that way but I have no problem with null first or last. Might take some time getting used to it though.
Reply
#14
(01-29-2016, 11:29 PM)pheonixstorm link Wrote: Tongue I ask for a code review and instead I get reviews of the thread and coding standards review.

As for block brace... meh.. seldom have I seen it used out of all the projects I have helped out. As for the null checks, up until working on the MM/HQ code I have never seen it done that way but I have no problem with null first or last. Might take some time getting used to it though.

Sorry, we did kind of get derailed there... didn't we? Big Grin

At any rate, I'm hoping to have time to look over "Time in Service" tomorrow.
Reply
#15
I actually do not want either of those.  For the null-first, I hadn't ever come across that until I saw it in a few places in Megamek.  I understand the idea behind it, but I don't like the way it looks (not a good argument, sure).  I also prefer the code formatting we have now.  Basically, it's a slightly modified variant of the default Eclipse code formatter.  I've gone back and forth between brace on the same line and brace on a new line, and I think I prefer brace on the same line (and I think that's the default in Java).
Reply
#16
(01-30-2016, 02:06 PM)Arlith link Wrote: I actually do not want either of those.  For the null-first, I hadn't ever come across that until I saw it in a few places in Megamek.  I understand the idea behind it, but I don't like the way it looks (not a good argument, sure).  I also prefer the code formatting we have now.  Basically, it's a slightly modified variant of the default Eclipse code formatter.  I've gone back and forth between brace on the same line and brace on a new line, and I think I prefer brace on the same line (and I think that's the default in Java).

Mostly we just need to come to a consensus on what the standards should be. Anyway, let's take that discussion to the dev forum Wink
Reply
#17
Since we're already derailed...

I definitely prefer brace on same line.

Null first I have no strong opinion on, though I'd call myself mildly in favor.

Not that I've contributed any code in probably close to a year now.
Reply
#18
Same line braces, that's been MM's style since the beginning. I don't care either way for null checks.
Reply


Possibly Related Threads…
Thread Author Replies Views Last Post
  Where do I find the repair bay code? pheonixstorm 12 5,116 08-01-2014, 12:59 AM
Last Post: ralgith
  Code - Large crew requirements? pheonixstorm 5 2,215 06-17-2014, 09:53 PM
Last Post: pheonixstorm
  Dropping zero minute techs - code questions pheonixstorm 5 2,497 06-09-2014, 04:26 AM
Last Post: pheonixstorm
  Time frame for next HQ release superdude 431 2 1,991 04-08-2012, 05:45 AM
Last Post: superdude 431

Forum Jump:


Users browsing this thread: 1 Guest(s)