Limit variable scope #10521

practicalswift commented at 9:41 PM on June 3, 2017: contributor

Limit variable scope as much as possible in order to:

improve code readability,
reduce complexity, and/or
avoid unnecessary initialisations.

practicalswift force-pushed on Jun 3, 2017

fanquake added the label Refactoring on Jun 4, 2017

sipa commented at 6:39 PM on June 4, 2017: member

utACK 5e6c94531046135d406ea755851fd614a76fb543

in src/policy/fees.cpp:858 in 5e6c945310 outdated

 854 | @@ -855,7 +855,7 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
 855 |      try {
 856 |          LOCK(cs_feeEstimator);
 857 |          int nVersionRequired, nVersionThatWrote;
 858 | -        unsigned int nFileBestSeenHeight, nFileHistoricalFirst, nFileHistoricalBest;
 859 | +        unsigned int nFileBestSeenHeight;

benma commented at 7:15 PM on June 4, 2017:

move down to the before L865, to move it closer to its first use.

in src/qt/trafficgraphwidget.cpp:50 in 5e6c945310 outdated

  47 | @@ -48,12 +48,12 @@ int TrafficGraphWidget::getGraphRangeMins() const
  48 |  void TrafficGraphWidget::paintPath(QPainterPath &path, QQueue<float> &samples)
  49 |  {
  50 |      int h = height() - YMARGIN * 2, w = width() - XMARGIN * 2;

benma commented at 7:19 PM on June 4, 2017:

h, w, x could also be moved into the if clause

in src/rpc/mining.cpp:106 in 5e6c945310 outdated

 103 |      int nHeight = 0;
 104 |  
 105 |      {   // Don't keep cs_main locked
 106 |          LOCK(cs_main);
 107 | -        nHeightStart = chainActive.Height();
 108 | +        int nHeightStart = chainActive.Height();

benma commented at 7:21 PM on June 4, 2017:

is this var even needed? could do nHeight = chainActive.Height(); and use nHeight instead of nHeightStart on the line after.

in src/script/interpreter.cpp:267 in 5e6c945310 outdated

 262 | @@ -263,11 +263,11 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
 263 |      set_error(serror, SCRIPT_ERR_UNKNOWN_ERROR);
 264 |      if (script.size() > MAX_SCRIPT_SIZE)
 265 |          return set_error(serror, SCRIPT_ERR_SCRIPT_SIZE);
 266 | -    int nOpCount = 0;
 267 |      bool fRequireMinimal = (flags & SCRIPT_VERIFY_MINIMALDATA) != 0;

benma commented at 7:23 PM on June 4, 2017:

why did you move only nOpCount but not this or some of the other vars above?

practicalswift commented at 8:50 PM on June 4, 2017:

Yes, there are more vars that could be moved into the tighter try scope. I'll take all interpreter.cpp changes in a separate PR since they'll need a more thorough review. I've now removed the nOpCount move from this PR.

benma changes_requested

practicalswift force-pushed on Jun 4, 2017

practicalswift commented at 8:51 PM on June 4, 2017: contributor

@benma Feedback items addressed! Thanks for the review.

in src/qt/trafficgraphwidget.cpp:53 in 4adc8389af outdated

  46 | @@ -47,13 +47,14 @@ int TrafficGraphWidget::getGraphRangeMins() const
  47 |  
  48 |  void TrafficGraphWidget::paintPath(QPainterPath &path, QQueue<float> &samples)
  49 |  {
  50 | -    int h = height() - YMARGIN * 2, w = width() - XMARGIN * 2;
  51 | -    int sampleCount = samples.size(), x = XMARGIN + w, y;
  52 | +    int sampleCount = samples.size();
  53 |      if(sampleCount > 0) {
  54 | +        int w = width() - XMARGIN * 2;
  55 | +        int x = XMARGIN + w, h = height() - YMARGIN * 2;

benma commented at 9:18 PM on June 4, 2017:

a super-nit, but I preferred w and h on the same line as before, because of the symmetry.

benma commented at 9:18 PM on June 4, 2017: none

utACK 4adc8389af964e84c1f97bc28707904341bbbc74

Limit variable scope 90593ed92c

practicalswift force-pushed on Jun 4, 2017

practicalswift commented at 10:53 PM on June 4, 2017: contributor

@benma Good point. w and h now on the same line again :-)

in src/qt/trafficgraphwidget.cpp:53 in 90593ed92c

  46 | @@ -47,13 +47,14 @@ int TrafficGraphWidget::getGraphRangeMins() const
  47 |  
  48 |  void TrafficGraphWidget::paintPath(QPainterPath &path, QQueue<float> &samples)
  49 |  {
  50 | -    int h = height() - YMARGIN * 2, w = width() - XMARGIN * 2;
  51 | -    int sampleCount = samples.size(), x = XMARGIN + w, y;
  52 | +    int sampleCount = samples.size();
  53 |      if(sampleCount > 0) {
  54 | +        int h = height() - YMARGIN * 2, w = width() - XMARGIN * 2;
  55 | +        int x = XMARGIN + w;

benma commented at 11:20 PM on June 4, 2017:

Sorry for another nit, but why not inline this x (YMARGIN + h is also inlined)? Then you can declare int x on line 56 (or inline those two as well).

Currently, there something like shadowing going on, where x is not modified inside the loop, as they are independent.

practicalswift commented at 6:50 PM on June 7, 2017:

@benma I'm not sure I follow. The value of x assigned in the loop is used outside of the loop, so declaring it on line 56 wouldn't maintain the same logic as before, would it? Please post a gist with your suggested formulation :-)

benma commented at 9:21 PM on June 8, 2017:

You are correct, sir! Apologies :)

practicalswift commented at 1:34 PM on June 9, 2017:

@benma Apology accepted! 😄

jtimon commented at 5:17 PM on June 5, 2017: contributor

utACK 90593ed92cfd5d49900a6fb6c2c10a482ab9fdbb

paveljanik commented at 4:43 AM on June 6, 2017: contributor

utACK 90593ed

sipa merged this on Jun 9, 2017

sipa closed this on Jun 9, 2017

sipa referenced this in commit 76f268b9bd on Jun 9, 2017

markblundeberg referenced this in commit 1b6adec5c1 on Jun 5, 2019

jtoomim referenced this in commit 603583a59c on Jun 29, 2019

jonspock referenced this in commit 2c1056d027 on Jul 4, 2019

PastaPastaPasta referenced this in commit 1fa3e9cd5f on Jul 5, 2019

proteanx referenced this in commit 4d52f0671d on Jul 5, 2019

PastaPastaPasta referenced this in commit 36c98d2529 on Jul 5, 2019

PastaPastaPasta referenced this in commit 06a40ad26d on Jul 6, 2019

PastaPastaPasta referenced this in commit c04a05f837 on Jul 8, 2019

PastaPastaPasta referenced this in commit f6400a8713 on Jul 8, 2019

jonspock referenced this in commit 0b4a16fa7a on Jul 9, 2019

barrystyle referenced this in commit 6d47f98363 on Jan 22, 2020

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 18, 2022