Mimic C#'s nameof() in Python I rewrote some code I posted earlier this month. It attempts to mimic C#'s nameof() by extracting names from bytecode instructions.

This seems more reliable than looking into the previous frame's locals and asserting that there is an id match between some value in the current scope. However, I'm unsure just how much because it uses dis:

  Use of this module should not be considered to work across Python VMs
  or Python releases.

I tried reducing the amount of bad input possible, but there is a quirky case where if I pass the function name_of((Cls.member(), var)) it returns 'member', if I remove the parentheses it will raise a SyntaxError because it no longer mistakes them for the end of the function call, and attribute access has been broken, so what the hell does it make of var.

import dis as _dis
from itertools import dropwhile as _dropwhile, takewhile as _takewhile

__all__ = ['name_of']

_LOAD = (
    'LOAD_CONST',
    'LOAD_NAME',
    'LOAD_ATTR',
    'LOAD_GLOBAL',
    'LOAD_FAST',
    'LOAD_CLOSURE',
    'LOAD_DEREF',
    'LOAD_CLASSDEREF'
)

class _Scopes:
    known = {}

def _dis_f(frame):
    return iter(_dis.Bytecode(frame.f_code))

def _known():
    def scope():
        return scope.cached[scope.no]

    scope.cached = []
    scope.no = 0
    return scope

def _not_eq(instruction):
    return instruction.argrepr != 'name_of'

def _in_load(instruction):
    return instruction.opname in _LOAD

def _invalidate(name):
    _Scopes.known.clear()
    raise SyntaxError('Invalid: {!r}'.format(name))

def _is_ident(name):
    if not name.isidentifier():
        _invalidate(name)

def _eq(instruction):
    return instruction.opname == _LOAD[2]

def _all_attr(instructions, name):
    if instructions[:-1] and not all(
            _eq(instruction) for instruction in instructions[1:]
    ):
        _invalidate(name)

def _cache(bytecode, scope):
    while True:
        instructions = _dropwhile(_not_eq, bytecode)
        if next(instructions, None) is None:
            break
        instructions = list(_takewhile(_in_load, instructions))
        name = instructions[-1].argrepr
        _is_ident(name)
        _all_attr(instructions, name)
        scope.cached.append(name)

def _pos(frame_id, scope):
    scope.no += 1
    if scope.no == len(scope.cached):
        del _Scopes.known[frame_id]

def name_of(_):
    try:
        raise NameError
    except Exception as exc:
        frame = exc.__traceback__.tb_frame.f_back
    frame_id = id(frame)
    if frame_id not in _Scopes.known:
        bytecode = _dis_f(frame)
        scope = _Scopes.known[frame_id] = _known()
        _cache(bytecode, scope)
    else:
        scope = _Scopes.known[frame_id]
    name = scope()
    _pos(frame_id, scope)
    return name

Overall it seems to work as intended, in both 3.4.4 and 3.6.3.

I'm not sure why you'd want to do this, however your code is really hard to understand.

You merge your functions together better.

name_of finds the parent frame, and then also goes on to handle creating and caching scope, along with finding the next name. This is defiantly breaking single responsibility.

I'd also merge all your one/two line functions into _cache. This is so we can tell what your code is actually doing.

What I'd do use:

the class for _Scopes,
a _scope function to build the scope,
a _get_frame function to get the current frame, and
the name_of function, to use _get_frame, and then call _Scopes.

To create _get_frame I'd move the try into it's own function.
To create _scope I'd:

Merge most of your code into _cache.
I manually perform the dropwhile and takewhile loops.
Move the code from _is_ident and _all_attr into the function.
Merge _all_attr with the takewhile loop. This is as you raise if i.opname in _LOAD. Unless it's LOAD_ATTR.
Remove the instructions list. It's not needed with the above, and you only want the last value anyway.
Don't mutate _Scopes from here.
Make this a generator of names, don't mutate anything.

To create _Scopes I'd:

Create a class, that inherits from collections.UserDict.
Move the rest of your name_of code into a next_name method.
You also can change the code so that it's better too.

Get the scope by using self.get. This means if it's not None then you can return without a second look-up.
Try to get the next(scope, None) if it's none, then you can implicitly remove the scope, by falling through to the creating code. Otherwise you can return the name.
If you wrap the call to _scope in a try, then you can perform the _Scopes.clear() function in the except clause.

And so I'd change your code to (untested):

import dis as _dis
from collections import UserDict

__all__ = ['name_of']

_ATTR = 'LOAD_ATTR'
_LOAD = set(
    'LOAD_CONST',
    'LOAD_NAME',
    'LOAD_GLOBAL',
    'LOAD_FAST',
    'LOAD_CLOSURE',
    'LOAD_DEREF',
    'LOAD_CLASSDEREF'
)

def _scope(frame):
    bytecode = iter(_dis.Bytecode(frame.f_code))
    while True:
        for instruction in bytecode:
            if instruction == 'name_of':
                break
        else:
            break

        name = None
        for instruction in bytecode:
            if instruction.opname != _ATTR:
                if instruction.opname not in _LOAD:
                    break
                raise SyntaxError('Invalid: {!r}'.format(name))
            name = instruction

        if name is None or not name.argrepr.isidentifier():
            raise SyntaxError('Invalid: {!r}'.format(name))
        yield name.argrepr

class _Scopes(UserDict):
    def next_name(self, frame):
        frame_id = id(frame)
        scope = self.get(frame_id, None)
        if scope is not None:
            name = next(scope, None)
            if name is not None:
                return name
        try:
            scope = self.data[frame_id] = _scope(frame)
        except SyntaxError:
            self.clear()
            raise
        return next(scope)

_scopes = _Scopes()

def _get_frame():
    try:
        raise Exception
    except Exception as e:
        return exc.__traceback__.tb_frame.f_back

def name_of(_):
    frame = _get_frame().f_back
    return _scopes.next_name(frame)


Guessing Game in Python Now that I've got this working, any improvements that I can make to better my code review would be helpful.

import random
import math
import console

console.set_font('Arial-BoldMT',15)

print'Welcome to the guessing game!\nA number will be randomly chosen from 0 to 1000.\nThe player will make a guess, and then the computer will guess. Who ever is closest wins that round!\nFirst to 5 wins!'

rounds = 0
run_again = 'y'
player_wins = 0
computer_wins = 0
draws = 0

while run_again == 'y':
    number = random.randint(0,1000)
    player_number = input('\nPlayer: ')
    computer_number = random.randint(0,1000)

    print 'Computer:', computer_number
    print 'Number:', number

    player_score = math.fabs(player_number - number)
    computer_score = math.fabs(computer_number - number)

    if player_score>=computer_score:
        computer_wins+=1
        console.set_color(1.00, 0.00, 0.00)
        print 'You lost that round'
        console.set_color(0.00, 0.00, 0.00)

    if player_score<=computer_score:
        player_wins+=1
        console.set_color(0.00, 0.00, 1.00)
        print 'You won that round'
        console.set_color(0.00, 0.00, 0.00)

    if player_score==computer_score:
        draws+=1
        console.set_color(0.00, 1.00, 0.00)
        print 'That round was a tie'
        console.set_color(0.00, 0.00, 0.00)

    rounds +=1

    if rounds == 5:
        if player_wins > computer_wins:
                console.set_color(0.00, 0.00, 1.00)
                print '\nYOU WON THE GAME'
                console.set_color(0.00, 0.00, 0.00)
                break
        elif computer_wins > player_wins:
                console.set_color(1.00, 0.00, 0.00)
                print '\nYOU LOST THE GAME'
                console.set_color(0.00, 0.00, 0.00)
                break
        else:
                print "Wrong counts"

I think the biggest thing here is that you need to create some functions and encapsulate stuff.

Big Points

Use reusable functions, especially when you find yourself copy-and-pasting code.
Don't hardcode anything; use constants rather than "magic numbers".
Don't use global variables.
Wrap it in a main().

Specifics

The minimum and maximum can be constants.

MINIMUM = 0
MAXIMUM = 1000

... Then, all you have to do is change these variables if you want to modify the game in the future:

number = random.randint(MINIMUM, MAXIMUM)
player_guess = input('\nPlayer: ')
computer_guess = random.randint(MINIMUM, MAXIMUM)

Break up the big multi-line message at the beginning rather than having it stretch on for 200 characters.  (Purists will still say that the implemenation below has the second line too long, as the Python standard is to have no line exceeding 80 characters, but you get the idea.)

print 'Welcome to the guessing game!'
print 'A number will be randomly chosen from ' + str(MINIMUM) + ' to ' + str(MAXIMUM)
print 'The player will make a guess, and then the computer will guess.'
print 'Whoever is closest wins that round!'
print 'First to 5 wins!'

You can have run_again directly store whether or not to run again (i.e., a boolean value) rather than a character.  This lets you check directly against it:

run_again = True
// ... then, elsewhere ...
if run_again:
    //do stuff

You can encapsulate the entire "guessing round" of the game into its own function which then returns whether or not the player won.  This will let you have a bit more control over your code structure:

def perform_guessing(number):
    player_guess = input('\nPlayer: ')
    computer_guess = random.randint(MINIMUM, MAXIMUM)
    print 'Computer: ' + str(computer_guess)
    print 'Number: ' + str(number)
    player_score = math.fabs(player_guess - number)
    computer_score = math.fabs(computer_guess - number)
    return player_score < computer_score

This will now return True if the player has won the round, so we can use it as such:

while run_again:
    if perform_guessing(random.randint(MINIMUM, MAXIMUM)):
        player_wins += 1
        console.set_color(0.00, 0.00, 1.00)
        print 'You won that round'
        console.set_color(0.00, 0.00, 0.00)
    else:
        computer_wins += 1
        console.set_color(1.00, 0.00, 0.00)
        print 'You lost that round'
        console.set_color(0.00, 0.00, 0.00)

(Note that in your code, it's actually impossible for a draw to occur, since you check both of the winning conditions with <= and >=, meaning that the == condition will be caught in one of those.  This may be a bug in your program. :) )

I might even make a color_print function to avoid some copy-and-pasting:

def color_print(text, r, g, b):
    console.set_color(r, g, b)
    print text
    console.set_color(0.00, 0.00, 0.00)

This would reduce our code above to:

while run_again:
    if perform_guessing(random.randint(MINIMUM, MAXIMUM)):
        player_wins += 1
        color_print('You won that round!', 0.0, 0.0, 1.0)
    else:
        computer_wins += 1
        color_print('You lost that round.', 1.0, 0.0, 0.0)

(I don't actually know what the three decimal values represent when you're setting the console color.  If they're not RGB, you can rename them accordingly.  But you get the idea.)

Finally, it's standard to encapsulate your functionality into a def main() and call it like so:

if __name__ == '__main__':
    main()

This lets you load you import things from this file into another file later without it actually executing.

Putting it all together, we get something that looks like this.  If it were me, I'd probably further specify the color_print function so that you could pass in something like Color.RED or Color.YELLOW rather than the three decimals, but that can be for a future release, haha.  There's some further encapsulation you could do to make the code still more elegant and readable, but I leave that as an exercise to the reader.

import random
import math
import console

MINIMUM = 0
MAXIMUM = 1000
FONT = 'Arial-BoldMT'
FONT_SIZE = 15

def perform_guessing(number):
    player_guess = input('\nPlayer: ')
    computer_guess = random.randint(MINIMUM, MAXIMUM)
    print 'Computer: ' + str(computer_guess)
    print 'Number: ' + str(number)
    player_score = math.fabs(player_guess - number)
    computer_score = math.fabs(computer_guess - number)
    return player_score < computer_score

def color_print(text, r, g, b):
    console.set_color(r, g, b)
    print text
    console.set_color(0.00, 0.00, 0.00)

def main():
    console.set_font(FONT, FONT_SIZE)
    print 'Welcome to the guessing game!'
    print 'A number will be randomly chosen from ' + str(MINIMUM) + ' to ' + str(MAXIMUM)
    print 'The player will make a guess, and then the computer will guess.'
    print 'Whoever is closest wins that round!'
    print 'First to 5 wins!'
    run_again = True
    player_wins = 0
    computer_wins = 0
    rounds = 0
    while run_again:
        rounds += 1
        if perform_guessing(random.randint(MINIMUM, MAXIMUM)):
            player_wins += 1
            color_print('You won that round!', 0.0, 0.0, 1.0)
        else:
            computer_wins += 1
            color_print('You lost that round.', 1.0, 0.0, 0.0)
        if rounds == 5:
            if player_wins > computer_wins:
                color_print('YOU WON THE GAME!', 0.0, 0.0, 1.0)
            else:
                color_print('YOU LOST THE GAME', 1.0, 0.0, 0.0)
            rounds = 0
            player_wins = 0
            computer_wins = 0

if __name__ == '__main__':
    main()


Optimize Recursive Function to traverse nested DOM nodes I wrote a recursive function that traverses nested DOM nodes of the following form:

The recursive function is the following:

function getNestedNodes(node) {
var ary = [];
for(var i = 0; i < node.length; ++i) {
    var myJSONChildren = {};
    if(node[i].childElementCount) {
        var myNode = node[i].children;
        for(var j = 0; j < myNode.length; ++j) {
            for(var k =0, attrs = myNode[j].attributes, l = attrs.length; k < l; ++k) {
                myJSONChildren['tag'] = myNode[j].nodeName;
                myJSONChildren[attrs.item(k).nodeName] = attrs.item(k).nodeValue;
            };
        }
        myJSONChildren['children'] = getNestedNodes(myNode); //Recursive Call
        ary.push(myJSONChildren);
    };
}
return ary;
}

so if I call that function this way:

var links = document.querySelectorAll('a');
getNestedNodes(node);

it returns a JSON array of the following form:

[{
  tag:'a',
  href:"#,
  title:"test",
  children:[{
             tag:"div",
             id:"nested-image",
             children:[{
                        tag:"img",
                        src:"image.jpg"
                       }]
             }]
  }]
}]

However, I think that my function is way too complex, and I know that there must a better way to get the same results.

Any help will be greatly appreciated!

Note that when you do document.querySelectorAll you are getting not a single node but a list of nodes. This seems to be causing confusion because your function refers to node.length - but a single node does not have any length property. Your original function also does not seem to behave exactly as you want it to in that it excludes the parent node (a in your example).

If you ensure that you are passing the function a single node, then it can indeed be simplified a lot:

function getTree(node) {
    var r = {tag: node.nodeName}, a, i;
    if (node.childElementCount) {
        r.children = [];
        for (i = 0; a = node.children[i]; i++) {
            r.children.push(getTree(a));
        }
    }
    for (i = 0; a = node.attributes[i]; i++) {
        r[a.nodeName] = a.nodeValue;
    }
    return r;
}
var links = document.querySelectorAll('a');
console.log(getTree(links[0])); // only pass the first node to the function

(JSfiddle here)


Check if an image download from the Internet has already started In the method getView() in my implementation of ArrayAdapter I have to download image from Internet using the image's url and set downloaded image to ImageView. I have to check in getView() whether downloading of an image from the given url is already started because of recycling views in TwoWayGridView and because of some bug there (immediately after creating a grid view the adapter's method getView() is called multiple times unnecessarily for the first item in grid view). To check that I use ImageView's tag.

@Override
public View getView(int position, View convertView, ViewGroup parent) {
    View itemView = (convertView != null) ? convertView : inflateItemView();
    ImageView imageView = (ImageView) itemView.findViewById(R.id.image);
    String url = getItem(position);
    setImageView(imageView, url);
    return itemView;
}

private void setImageView(ImageView imageView, String url) {
    if (url == null) {
        imageView.setImageDrawable(null);
    } else {
        downloadAndSetImage(imageView, url);
    }
}

private final Object LOCK_CHECKING_TAG_AND_URL = new Object();

private void downloadAndSetImage(final ImageView imageView, final String url) {
    synchronized (LOCK_CHECKING_TAG_AND_URL) {
        int tagId = R.id.tag_prevent_unnecessary_image_reset;
        String tag = (String) imageView.getTag(tagId);
        if (tag != null && tag.equals(url)) {
            // Image downloading from the given url for this ImageView is already started.
            // There is no need to start new downloading.
            return;
        }
        imageView.setTag(tagId, url);
    }
    imageView.setImageDrawable(null);
    // ImageManager is my utility class for downloading images in background
    imageManager.getImage(imageView, url, new ImageManager.Listener() {
        @Override
        public void imageReady(Drawable drawable) {
            imageView.setImageDrawable(drawable);
        }
    });
}

Later I changed the downloadAndSetImage() method:

private void downloadAndSetImage(final ImageView imageView, final String url) {
    if (!isNeedToDownload(imageView, url)) {
        return;
    }
    imageView.setImageDrawable(null);
    imageManager.getImage(imageView, url, new ImageManager.Listener() {
        @Override
        public void imageReady(Drawable drawable) {
            imageView.setImageDrawable(drawable);
        }
    });
}

private boolean isNeedToDownload(View imageView, String url) {
    synchronized (LOCK_CHECKING_TAG_AND_URL) {
        int tagId = R.id.tag_prevent_unnecessary_image_reset;
        String tag = (String) imageView.getTag(tagId);
        if (tag != null && tag.equals(url)) {
            return false;
        }
        imageView.setTag(tagId, url);
        return true;
    }
}

But I think that the name isNeedToDownload() is bad (or may be the method itself is bad) because this boolean checking method not only checks some condition but also set a tag to ImageView. I think that the name like a checkDownloadingNecessity() and return type

class CheckingResult {
    public boolean isDownloadingNecessary;
}

would be better, but it is too excessively. What do you think?

Just a note:

... and return type

class CheckingResult {
    public boolean isDownloadingNecessary;
}

Please don't. An encapsulated boolean goes really too far. Especially on Android as an empty class costs 500 bytes. The method is probably fine, the name could be improved to something less boolean-like. You could also use a ConcurrentSet like

private final Set<imageView> startedDownloads = Sets.newConcurrentHashSet();

private boolean registerDownload(View imageView, String url) {
    return startedDownloads.add(imageView);
}

This assumes Guava, but without it, you could use a ConcurrentHashMap with a dummy value instead. No idea what everything your LOCK_CHECKING_TAG_AND_URL gets used for, so my replacement may not be equivalent. I also removed your tagging in favor of placing the View itself in the set, which may be completely wrong. But why did you chose to use tags instead?

With whatever reason, this should be explained.

If it was because of memory, you could use a WeakReference<View> (with a delegating equals, which makes it too complicated).


Math Skills Game Advice I think my code works pretty well, although I'm biased: http://jsfiddle.net/AHKb4/2/
Basic Overview: I'm working on building a math skill game, where the objective is to drag and drop div's to a container. Each div will have a value, likely set with classes. Each container must equal theSumNum to pass the level. Just giving this info so you can possibly understand better what I'm trying to achieve.

Most importantly I think I've got my main function pickNumbers() ready to go. But I'd really appreciate any opinions and advice that you have for that function.

I've made two classes so far, three and five. I'm planning though to have classes from two through ten. I'm thinking each class will have an amount of arrays equal to that class number. Likewise there will be x number of container divs for x number of arrays.
So I think I could obviously improve how I do my classes by creating a for loop to generate the classes when needed, and use like myArray1 instead of myArrayOne.  I'm just looking for a True or False answer for this question.

Lastly, please take a look at the function pickFive(). So again I'm assuming I could just use a for loop for that as well? Just looking for a True or False here too, but if there's anything to look out for or be careful of in doing it that way, please let me know.

$(document).ready(function() {
var three = {
    myArrayOne: [],
    myArrayTwo: [],
    myArrayThree: [],
    myArraysCombined: []
}
var five = {
    myArrayOne: [],
    myArrayTwo: [],
    myArrayThree: [],
    myArrayFour: [],
    myArrayFive: [],
    myArraysCombined: []
}
// theArray is the array to push to  ~ ex(three.myArrayTwo)
// theNum is the amount of numbers that will be in the array  ~ ex(3)
// theSumNum is what the sum of all the numbers in the array will equal  ~ ex(20)
// theMaxNum is the largest number that can appear in the array, except the last number may be larger since its not random  ~ ex(10)
// theMinNum is the smallest number that can appear in the array  ~ ex(1)
// aRandNum is a temporary var for each number in the array, except for the last number in the array
// thePreSumNum is the sum of all the numbers in the array except for the last number
// theLastNum is the last number that will be pushed to the array, this number is not random
function pickNumbers(theArray, theNum, theSumNum, theMaxNum, theMinNum) {
    for (var x=0; x < theNum;) {
        var aRandNum = Math.floor(Math.random()*theMaxNum + theMinNum);
        if (theArray.indexOf(aRandNum) === -1) {
            if (theArray.length < (theNum - 1)) {
                theArray.push(aRandNum);
                x += 1;
            }
            if (theArray.length === (theNum -1)) {
                var thePreSumNum = 0;
                for (var e = 0; e < theArray.length; e += 1) { thePreSumNum += theArray[e]; }
                var theLastNum = theSumNum - thePreSumNum;
                    if (theArray.indexOf(theLastNum) > -1 || theLastNum < 1) {
                        theArray.length = 0;
                        x = theNum;
                        pickNumbers(theArray, theNum, theSumNum, theMaxNum, theMinNum);
                    }
                    else {
                        theArray.push(theLastNum);
                        x += 1;
                    }
            }
        }
    }
}

function pickThree() {
    pickNumbers(three.myArrayOne, 3, 20, 10, 1);
    pickNumbers(three.myArrayTwo, 3, 20, 10, 1);
    pickNumbers(three.myArrayThree, 3, 20, 10, 1);
    three.myArraysCombined = three.myArrayOne.concat(three.myArrayTwo, three.myArrayThree);
}

function pickFive() {
    pickNumbers(five.myArrayOne, 5, 40, 20, 1);
    pickNumbers(five.myArrayTwo, 5, 40, 20, 1);
    pickNumbers(five.myArrayThree, 5, 40, 20, 1);
    pickNumbers(five.myArrayFour, 5, 40, 20, 1);
    pickNumbers(five.myArrayFive, 5, 40, 20, 1);
    five.myArraysCombined = five.myArrayOne.concat(five.myArrayTwo, five.myArrayThree, five.myArrayFour, five.myArrayFive);
}

pickThree();
pickFive();

});

Assumptions I made:

The 3 object will have 3 arrays, just as the 67 object will have 67 arrays
The "combined" array can be computed on demand
Each call to pickNumbers for a certain object will always have the same parameters

Firstly for your objects, I think they belong in a namespace and also a way to define which numbers you want to be in the game:

var Game = {}
Game.NUMBERS = [3, 5];

Next instantiate all the classes:

for (var i = Game.NUMBERS.length; i --) {
  Game[Game.NUMBERS[i]] = new GameNumber(Game.NUMBERS[i]);
}

Let's define what your class would look like:

var GameNumber = function(number) {
  this.number = number;
  this.arrays = [];
  for (var i = number; i --) {
    this.arrays.push([])
  }
}
GameNumber.prototype.combined = function() {
  Array.prototype.concat.apply([], this.arrays)
}
GameNumber.prototype.pick = function(arrayLength, sum, max, min) {
  for (var i = this.number; i --) {
    pickNumbers(this.arrays[i], arrayLength, sum, max, min);
  }
}

Cool, so now you can create your objects in a sufficiently general way and call pick or combined on any of them.

Game[3].pick(5, 40, 20, 1);
Game[3].combined() // Output: [...]

As for the pickNumbers function, I didn't look at the logic too heavily, I'm sure that's working how you want it to. The variable names I would simplify to array, arrayLength, sum, max, min for readability. You may also want to attach it to the Game object so you have less globals (ie Game.pickNumbers = function(...)). I'd also try and abstract the html from it so as to separate logic from presentation.

Note: I haven't tested this code, it's only to give you an idea of how to structure it.


Function to get title based on pathname this code seems fine at once but the problem I see with this code maintainability I can imagine if one will have to add 10 or more routes the function will grow badly.
can we refactor this code for better maintainibility .
export const getMetaDescription = (pathname) => {
  if (pathname.includes("about")) return "About Us | " + BASE_SITE_NAME;
  else if (pathname.includes("fees")) return "Our Fees | " + BASE_SITE_NAME;
  else if (pathname.includes("legal")) return "Legal Info | " + BASE_SITE_NAME;
  else if (pathname.includes("register"))
    return "Registration | " + BASE_SITE_NAME;
  else if (pathname.includes("login")) return "Login | " + BASE_SITE_NAME;
  return BASE_SITE_NAME;
};

Edit:
i had a similar function which i refactor by using a map (kind of)
const titles = {
  about: "About Us",
  login: "Login",
  wallet: "Wallet",
  profile: "Profile",
  beneficiary: "Beneficiary",
};

export const getMetaTitle = pathname => {
  for (let key in titles) {
    if (pathname.includes(key)) {
      return titles[key] + " | " + BASE_SITE_NAME;
    }
  }
  return BASE_SITE_NAME;
};

I'd make an object whose key is the pathname and whose value is the unique string for that pathname, then iterate through the object to find the matching key:

const BASE_SITE_NAME = 'BASE_SITE_NAME';
const descriptions = {
    about: 'About Us',
    fees: 'Our Fees',
    legal: 'Legal Info',
    register: 'Registration',
    login: 'Login',
};
const getMetaDescription = (pathname) => {
    const foundDescriptionKey = Object.keys(descriptions).find(key => pathname.includes(key));
    return foundDescriptionKey
        ? descriptions[foundDescriptionKey] + ' | ' + BASE_SITE_NAME
        : BASE_SITE_NAME;
};

console.log(getMetaDescription('about'));
console.log(getMetaDescription('foobar'));

If pathname is directly from the URL in the browser, make sure to call toLowerCase() on it first (just in case, for example, someone types in example.com/ABOUT instead of clicking a link that redirects to /about).
This is faithful to your original code. But if each of the descriptions correspond to an entirely separate route, it'd be better to just use plain property lookup. For example, if the pathname is /about, extract the about from that and look up that key on the object, instead of iterating.

const BASE_SITE_NAME = 'BASE_SITE_NAME';
const descriptions = {
    about: 'About Us',
    fees: 'Our Fees',
    legal: 'Legal Info',
    register: 'Registration',
    login: 'Login',
};
const getMetaDescription = (pathname) => {
    const description = descriptions[pathname.slice(1)];
    return description
        ? description + ' | ' + BASE_SITE_NAME
        : BASE_SITE_NAME;
};

console.log(getMetaDescription('/about'));
console.log(getMetaDescription('/foobar'));

The .includes approach is needed only if you have multiple pathnames associated with a single description. Even if you have multiple pathnames associated with a single description, consider using startsWith instead - it'll be more precise and less likely to have collisions. For example, if you have example.com/profile/aboutme, the about would result in About Us being displayed, which would not be desirable.

const BASE_SITE_NAME = 'BASE_SITE_NAME';
const descriptions = {
    about: 'About Us',
    fees: 'Our Fees',
    legal: 'Legal Info',
    register: 'Registration',
    login: 'Login',
    profile: 'Profile',
};
const getMetaDescription = (pathname) => {
    const foundDescriptionKey = Object.keys(descriptions).find(key => pathname.slice(1).startsWith(key));
    return foundDescriptionKey
        ? descriptions[foundDescriptionKey] + ' | ' + BASE_SITE_NAME
        : BASE_SITE_NAME;
};

console.log(getMetaDescription('/about'));
console.log(getMetaDescription('/profile/aboutme'));
console.log(getMetaDescription('/foobar'));


Storage class, dependency injection and singletons In re-writing my PHP framework vervPHP, I've created the following class:

class storage {
    private static $instance;
    public $db = null; // Holds a database connection object
    public $user = null; // Holds user related information for custom auth
    public $data = array(); // Array of data for storage

    private function __construct(){
        // Create the default array in $this->data....
    }

    public static function singleton() {
        if (!isset(self::$instance)) {
            $className = __CLASS__;
            self::$instance = new $className;
        }
        return self::$instance;
    }

    // Getters, Setters and other stuff follows...
}

When my framework starts up, it looks like this:

index.php:

// Kick it off!
verv\init();

framework/verv/verv.php:

function init() {
    $verv = storage::singleton();

    loadConfig($verv);
    loadRequest($verv);
    connectDB($verv);
    authenticateUser($verv);
    loadModule($verv);
    loadTemplate($verv);
    renderPage($verv);
}

$verv is passed by reference - so any changes are saved as it moves around.

My question is, do I need the singleton portion? And basically, am I doing it "right"? The code in question works (albeit it needs a little tidying) but I want to make sure that my storage class is as robust and efficient as possible.

Read this post before using the singleton pattern:  https://stackoverflow.com/a/4596323/1908639

You might find that it increases your initial development speed; however, the downsides tend to increase with the size/complexity of the project.  I have personally maintained code that used singletons, and I found it difficult to test/reuse.

If you think there are any major advantages to the singleton approach, I would be interested in hearing them.  I have not been able to find any beneficial use-cases in PHP web development, but I am definitely interested in hearing about them, if they exist.

Also, passing objects by reference should not be necessary in PHP 5 (source):

  In PHP 5 there is a new Object Model. PHP's handling of objects has been completely rewritten, allowing for better performance and more features. In previous versions of PHP, objects were handled like primitive types (for instance integers and strings). The drawback of this method was that semantically the whole object was copied when a variable was assigned, or passed as a parameter to a method. In the new approach, objects are referenced by handle, and not by value (one can think of a handle as an object's identifier).


How to improve execution of web service using System.DirectoryServies.AccountManagement that runs very slow? This method runs in just under two minutes.  I would like to optimize it to run in less than 15 seconds. Using a LAMDA filter on my list before iterating it's elements and removing one conditional statement in the method shaved off 30 seconds.  Any ideas to improve performance?

c# .net 4.0

[SecurityCritical]
        [SecurityPermissionAttribute(SecurityAction.Demand)]
        private static void GetGroupMembership(List<ActiveDirectoryPrincipalProperties> userGroupProperties)
        {
            List<ActiveDirectoryPrincipalProperties> groupProperties = new List<ActiveDirectoryPrincipalProperties>();

            foreach (ActiveDirectoryPrincipalProperties gProperties in userGroupProperties.FindAll(token => token.groupYesNo.Equals(true)))
            {

                PrincipalContext ctx = new PrincipalContext(ContextType.Domain, gProperties.groupDomain);
                try
                {

                    GroupPrincipal group = GroupPrincipal.FindByIdentity(ctx, IdentityType.Name, gProperties.groupName);

                    foreach (Principal member in group.GetMembers(true))
                    {
                        ActiveDirectoryPrincipalProperties memberProperties = new ActiveDirectoryPrincipalProperties();
                        memberProperties.fullGroupName = gProperties.fullGroupName;
                        memberProperties.groupDomain = gProperties.groupDomain;
                        memberProperties.groupName = gProperties.groupName;
                        memberProperties.groupType = gProperties.groupType;
                        memberProperties.groupYesNo = false;
                        memberProperties.memberDomain = member.Context.Name.ToString();
                        memberProperties.memberName = member.SamAccountName.ToString();
                        memberProperties.memberType = member.StructuralObjectClass.ToString();
                        memberProperties.sqlUserOnlyYesNo = false;

                        groupProperties.Add(memberProperties);
                    }
                    group.Dispose();
                }
                finally
                {
                    ctx.Dispose();
                }

            }

            userGroupProperties.AddRange(groupProperties);
        }

If you are writing this in Visual Studio 2010, it would be worth profiling your code to find out exactly what method calls take the longest.

Here is a link where you can watch a video from Channel9 about using the built in performance analyser tool in Visual Studio 2010 to perform CPU Sampling on your code: http://channel9.msdn.com/Blogs/wriju/CPU-Sampling-using-Visual-Studio-2010-Performance-Analyzer-Tool

Once you've identified the methods that take the longest to execute, you can start to work out if you're making any redundant or excessive calls and remove them, or research each call to find ways of optimising each.


Elegant Method for Sleeping with Selenium Webscraper My goal is to scrape the no. 1 suggestion of the suggestion list on https://finance.yahoo.com for any entry. If there is no suggestion the code should write f"nan_{what_ever_the_entry_was}".

As basis I used the solution from https://stackoverflow.com/questions/50355120/how-to-scrape-the-yahoo-finance-search-auto-suggestion-result-with-selenium-pyth/60496531#60496531.

Since I want to scrape the first suggestion for various values in a list, I created a for-loop. If I had kept the third to last line of the original solution

WebDriverWait(driver, 5).until(EC.element_to_be_clickable((By.XPATH, "//input[@name='yfin-usr-qry']"))).send_keys("goog")

the function won't work properly, because the elements are already loaded for the second loop. That's why I added two time.sleep() blocks instead. Now the function works well.

Question

The time.sleep() part does not seem very elegant to me. Is this the best way or are there more elegant/pythonic ways to resolve the problem?

Input

from typing import List
import time
from selenium import webdriver
from selenium.webdriver.chrome.options import Options
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.common.by import By
from selenium.webdriver.support import expected_conditions as EC

options = Options()
options.add_argument("start-maximized")
options.add_argument("disable-infobars")
options.add_argument("--disable-extensions")
options.add_argument('headless')

def get_ticker_symbols(isin_list: List) -> List:
    driver = webdriver.Chrome(chrome_options=options, executable_path=r'C:\Program Files (x86)\Google\Chrome\Chromedriver\chromedriver.exe')

    driver.get('https://finance.yahoo.com/')
    driver.find_element_by_xpath("//button[@type='submit' and @value='agree']").click()

    ticker_list=[]
    for isin in isin_list:
        try:
            WebDriverWait(driver, 5).until(EC.element_to_be_clickable((By.XPATH, "//input[@name='yfin-usr-qry']"))).send_keys(f"{isin}")
            time.sleep(1)
            ticker = driver.find_element_by_xpath('(//div[@class="_0ea0377c _4343c2a0 _50f34a35"])[1]').text

        except:
            ticker = f'nan_{isin}'
            time.sleep(0.1)

        finally:
            ticker_list.append(ticker)
            driver.find_element_by_xpath("//input[@name='yfin-usr-qry']").clear()

    return ticker_list

get_ticker_symbols(['GB00BD2ZT390','bmw','dasdas', 'nike', 'GB00BK8FL363'])

Output

['GPH.L', 'BMW.DE', 'nan_dasdas', 'NKE', 'HZD.L']

WebDriverWait

Your intuition about time.sleep() not being ideal here is correct. A better alternative is to use the Selenium WebDriverWait API to wait for elements of interest to appear/disappear. More specifically,

after entering a query in the query field, we want to wait for the list elements to show up in the DOM so we can retrieve the first element
after clearing the query field, we want to wait for any of the aforementioned list elements to disappear from the DOM, as a sanity check to ensure we've reset the page to a state that is ready for our next query

For waiting for the elements to show up in the DOM, we can use presence_of_all_elements_located:

first_element = WebDriverWait(driver, 5).until(
    EC.presence_of_all_elements_located(
        (By.XPATH, "//div[@class='_0ea0377c _4343c2a0 _50f34a35']")
    )
)[0]
ticker = first_element.text

For waiting for any of the above elements to disappear from the DOM, we can use staleness_of or invisibility_of_element_located:

WebDriverWait(driver, 5).until(EC.staleness_of(first_element))

# also works, but we're repeating ourselves a bit here
WebDriverWait(driver, 5).until(
    EC.invisibility_of_element_located(
        (By.XPATH, "//div[@class='_0ea0377c _4343c2a0 _50f34a35']")
    )
)

Query field: retrieve once then reuse

  for isin in isin_list:
    try:
        WebDriverWait(driver, 5).until(EC.element_to_be_clickable((By.XPATH, "//input[@name='yfin-usr-qry']"))).send_keys(f"{isin}")

We only need to wait for the search query field to become clickable once. So we can move the WebDriverWait line outside of the loop and save its return value (reference to the query field element) in query_field:

query_field = WebDriverWait(driver, 5).until(
    EC.element_to_be_clickable((By.XPATH, "//input[@name='yfin-usr-qry']"))
)
for query in queries:
    try:
        query_field.send_keys(query)
    # ...

Incorrect assumption about modal always appearing

  driver.find_element_by_xpath("//button[@type='submit' and @value='agree']").click()

This appears to handle a pop-up modal by clicking past it, but I don't think the modal shows up all the time. When I ran the original script the above line threw a NoSuchElementException.

To handle the case when the modal doesn't appear, ignore NoSuchElementException when it is raised from that line:

try:
    driver.find_element_by_xpath(
        "//button[@type='submit' and @value='agree']"
    ).click()
except NoSuchElementException:
    pass

Use None to represent the absence of a value

  If there is no suggestion the code should write f"nan_{what_ever_the_entry_was}".

I would highly recommend against doing this. Instead, use None to represent the absence of a value (see typing.Optional).

get_ticker_symbols would then return a List[Optional[str]], which is a clear way of expressing in the function signature that it is possible for a search query to return an "empty" result.

As for the current function signature,

  def get_ticker_symbols(isin_list: List) -> List:

this can be improved by specifying the types of elements in each list:

def get_ticker_symbols(queries: List[str]) -> List[Optional[str]]:

Also, isin_list is not a very descriptive name and it almost sounds like a boolean, which is misleading, so I'd go with something like queries instead.

Specify the exceptions you are handling

Avoid using bare except clauses, which catch all exceptions and can obscure/hide real issues or bugs. Instead, specify the exception you are handling:

try:
    # some code that could throw a `TimeoutException`,
    # e.g. `presence_of_all_elements_located`
except TimeoutException:
    # ...

Close resources after use

It's a good idea to call close() or quit() on the WebDriver to exit the browser once we're done using it. That said, if we invoke the WebDriver as a context manager, we don't have to worry about this:

with webdriver.Chrome(options=options) as driver:
    driver.get("https://finance.yahoo.com/")
    # ...

Selenium allows us to invoke the WebDriver as a context manager as shown above, which means we don't need to call driver.close() or driver.quit() explicitly because it will be done for us when the flow of execution leaves the indented block.

Refactored version

#!/usr/bin/env python3

from typing import List, Optional

from selenium import webdriver
from selenium.common.exceptions import NoSuchElementException, TimeoutException
from selenium.webdriver.chrome.options import Options
from selenium.webdriver.common.by import By
from selenium.webdriver.common.keys import Keys
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import WebDriverWait

options = Options()
options.add_argument("start-maximized")
options.add_argument("disable-infobars")
options.add_argument("--disable-extensions")
options.add_argument("headless")

def get_ticker_symbols(queries: List[str]) -> List[Optional[str]]:
    with webdriver.Chrome(options=options) as driver:
        driver.get("https://finance.yahoo.com/")

        try:
            driver.find_element_by_xpath(
                "//button[@type='submit' and @value='agree']"
            ).click()
        except NoSuchElementException:
            pass

        query_field = WebDriverWait(driver, 5).until(
            EC.element_to_be_clickable(
                (By.XPATH, "//input[@name='yfin-usr-qry']")
            )
        )
        ticker_list = []
        for query in queries:
            try:
                query_field.send_keys(query)
                first_element = WebDriverWait(driver, 5).until(
                    EC.presence_of_all_elements_located(
                        (
                            By.XPATH,
                            "//div[@class='_0ea0377c _4343c2a0 _50f34a35']",
                        )
                    )
                )[0]
                ticker = first_element.text
            except TimeoutException:
                ticker = None

            ticker_list.append(ticker)
            # web_element.clear() doesn't fire any keyboard events, and
            # as a result the change to the text field isn't registered
            # as we would expect. So as a workaround, we delete the query
            # via Keys.BACKSPACE
            query_field.send_keys(Keys.BACKSPACE * len(query))
            WebDriverWait(driver, 5).until(EC.staleness_of(first_element))

    return ticker_list

if __name__ == "__main__":
    queries = ["GB00BD2ZT390", "bmw", "dasdas", "nike", "GB00BK8FL363"]
    print(get_ticker_symbols(queries))

Alternative solution: Use undocumented API

This is a bit out of scope for this code review, but I do think it deserves a mention. Simply put, there is an even better way of doing all of this without using Selenium or other web scraping libraries.

Using something like Chrome Developer Tools, we can track the XHRs made when typing in the search field and figure out which API call is providing the data we care about.

In this case it's an undocumented API https://query1.finance.yahoo.com/v1/finance/search which we can call directly with requests:

#!/usr/bin/env python3

import requests

from typing import Any, Dict, Optional, List

def get_ticker_symbols(queries: List[str]) -> List[Optional[str]]:
    def get_params(query: str) -> Dict[str, Any]:
        return {
            "q": query,
            "lang": "en-US",
            "region": "US",
            "quotesCount": 1,
            "newsCount": 0,
            "enableFuzzyQuery": False,
        }

    ticker_list = []
    with requests.Session() as session:
        for query in queries:
            response = session.get(
                "https://query1.finance.yahoo.com/v1/finance/search",
                params=get_params(query),
            )
            response.raise_for_status()
            quotes = response.json()["quotes"]
            ticker_symbol = quotes[0]["symbol"] if quotes else None
            ticker_list.append(ticker_symbol)

    return ticker_list

if __name__ == "__main__":
    queries = ["GB00BD2ZT390", "bmw", "dasdas", "nike", "GB00BK8FL363"]
    print(get_ticker_symbols(queries))


PermMissingElem- find the missing element in a given permutation in C# I just encountered this question in codility but apparently this question has been asked in java already  (here, here).

Task description

A zero-indexed array A consisting of \$N\$ different integers is given. The array contains integers in the range \$[1..(N + 1)]\$, which means that exactly one element is missing.

Your goal is to find that missing element.

Write a function:

class Solution { public int solution(int[] A); }

that, given a zero-indexed array A, returns the value of the missing element.

For example, given array A such that:

  A[0] = 2
  A[1] = 3
  A[2] = 1
  A[3] = 5

the function should return 4, as it is the missing element.

Assume that:

  \$N\$ is an integer within the range [0..100,000];

  the elements of A are all distinct; each element of array A is an
  integer within the range \$[1..(N + 1)]\$.

Complexity:

  expected worst-case time complexity is \$O(N)\$;
  expected worst-case space complexity is \$O(1)\$, beyond input storage (not counting the storage required for input arguments).
  Elements of input arrays can be modified.

My approach is shown below:

  public static int permMissingElement(int [] elements)
            {
                if (elements.Length == 0)
                {
                return 0;
            }

            else if (elements.Length == 1)
            {
                return elements.First();
            }
            else
            {
                Array.Sort(elements);
                List<int> listOfElementsInTheArray = elements.ToList<int>();
                IEnumerable<int> missingNumber = Enumerable.Range(listOfElementsInTheArray.First(), listOfElementsInTheArray.Count).Except(listOfElementsInTheArray);

               return missingNumber.Count() == 0 ? 0 : missingNumber.First();

            }
        }

 static void Main(string[] args)
        {
            Console.WriteLine(permMissingElement(new int [] {2,3,1,5}));
            Console.ReadLine();
        }

Explanation: My approach was to display 0 if the array doesn't contain an element and return the element if it contains 1 . In addition, if the array contains 2 or more elements, it is sorted and converted to a list. Once its a list , i generate a complete sequence of the same list and remove any list that exists in the previous. I return the missing number afterwards . But surprising codility grades me in terms of correctness 20% and performance 80% and in general 50%. I know this can be better improved, any suggestions would be appreciated.

It is possible to solve the problem without sorting the array (which is expensive). The input array is actually a simple arithmetic sequence 1, 2, 3, 4 ... N+1, the numbers are just in a jumbled up order and one of them is missing.

The sum of the sequence is easy to calculate as n/2(1+n) (here n = N + 1). The sum of the input array will be the same as this, minus the value of the missing element. So the value of the missing element is the difference between the sum of the entire sequence and the sum of the input array.

    public static int permMissingElement(int[] elements)
    {
        int n = elements.Length + 1;
        int sumOfAllElements = (n * (1 + n)) / 2;
        int missingElement = sumOfAllElements - elements.Sum();
        return missingElement;
    }

Edit:

The maximum value of N is 100,000, which would cause an arithmetic overflow when calculating the sum of the sequence. The straightforward solution is to use longs for the calculations instead:

    public static int permMissingElement(int[] elements)
    {
        long n = elements.Length + 1;
        var sumOfAllElements = (n * (1 + n)) / 2;
        var missingElement = sumOfAllElements - elements.Select(x => (long)x).Sum();
        return (int)missingElement;
    }


Python implementation of Quicksort using list comprehensions I wrote an implementation of quicksort that I think is pythonic. I based it off of this common Haskell implementation:

  quicksort :: (Ord a) => [a] -> [a]
quicksort [] = []
quicksort (x:xs) =
    let smallerSorted = quicksort [a | a <- xs, a <= x]
        biggerSorted = quicksort [a | a <- xs, a > x]
    in  smallerSorted ++ [x] ++ biggerSorted

I understand this is not an in-place sort. I also understand that it is optimized for Haskell, which handles recursion very well.

Here's my Python adaptation:

def quicksort(li):
    if li == []:
        return li
    p = li[0]
    lo = [ i for i in li[1:] if i <= p ]
    hi = [ i for i in li[1:] if i > p ]
    return quicksort(lo) + [p] + quicksort(hi)

I think it has the following upsides:

clean looking, pythonic
fast C implementation of linear partitioning

and the following downsides:

does not sort in place
possibly memory inefficient

What are some issues I should be aware of besides the above? Can the algorithm be improved in terms of memory complexity without sacrificing the simplicity and pythonic structure? Should I avoid recursion altogether? Does anyone see bugs or have proposed changes?

IMHO you can't really compare in-place quicksort to not-in-place quicksort. While the basic algorithm is the same, the implementation is vastly different, one being in-place (d'uh) and the other being much, much simpler.

Having said that, you could make your code a bit faster and (IMHO) cleaner by a) using wildcard-unpacking to separate the pivot from the rest (no need to slice the list twice), and b) return the list itself if it contains zero or one elements (no need to partition a one-elemented list).

def quicksort(lst):
    if len(lst) <= 1:
        return lst
    else:
        p, *rest = lst
        low  = [x for x in rest if x <= p]
        high = [x for x in rest if x >  p]
        return quicksort(low) + [p] + quicksort(high)

The else is kind of redundant here, but IMHO it's cleaner that way.

However, using the first element of the list as pivot can be problematic if the list is already (partially) sorted, which is not uncommon. In this case, the performance will be much better/more predictable using a randomly chosen pivot (or any other method). This means that we have to partition the list into three parts, though, low, med, and high, otherwise we risk getting infinite recursion in case the list contains two or more copies of the pivot.

def quicksort(lst):
    if len(lst) <= 1:
        return lst
    else:
        p = random.choice(lst)
        low  = [x for x in lst if x <  p]
        med  = [x for x in lst if x == p]
        high = [x for x in lst if x >  p]
        return quicksort(low) + med + quicksort(high)


Inserting a node to a given position in a linked list Given

  You’re given the pointer to the head node of a linked list, an integer to add to the list and the position at which the integer must be inserted. Create a new node with the given integer, insert this node at the desired position and return the head node. A position of 0 indicates head, a position of 1 indicates one node away from the head and so on. The head pointer given may be null meaning that the initial list is empty.

Solution

Node InsertNth(Node head, int data, int position) {
    Node newNode = new Node();
    newNode.data = data;
    newNode.next = null;

    if (head == null) {
        return newNode;
    }
    if (position == 0) {
        newNode.next = head;
        head = newNode;
        return head;
    }
    Node prev = null;
    Node current = head;
    int i = 0;
    while (current !=null && i < position) {
        prev = current;
        current = current.next;
        i++;
    }
    newNode.next = prev.next;
    prev.next = newNode;
    return head;
}

The code works fine. But I strongly feel that there is a chance of improvement. I see a lot of checks here, how can I reduce those and make my code more readable and covering all the edge cases?

Also, is there any recursive version possible for the given code because list is a recursive data structure hence, there should be some recursive way, right?

I see a lot of checks here, how can I reduce those and make my code more readable and covering all the edge cases?

Using a dummy node that points to the head can help reduce many of the checks:

Node insertNth(Node head, int data, int position) {
    Node dummy = new Node();
    dummy.next = head;

    Node runner = dummy;
    for (int i = 0; i < position; ++i) {
        runner = runner.next;
    }

    Node node = new Node();
    node.data = data;
    node.next = runner.next;
    runner.next = node;

    return dummy.next;
}

  Also, is there any recursive version possible for the given code because list is a recursive data structure hence, there should be some recursive way, right?

Naturally, a recursive solution is also possible:

Node insertNthRecursive(Node head, int data, int position) {
    if (position == 0) {
        Node node = new Node();
        node.data = data;
        node.next = head;
        return node;
    }
    head.next = insertNthRecursive(head.next, data, position - 1);
    return head;
}

Update

As @PeterCordes pointed out, the above implementation is a bit dirty,
as it rewrites the links unnecessarily.
Here's a variant replacing the head.next = ... line above to make it cleaner.

    Node next = insertNthRecursive(head.next, data, position - 1);
    if (position == 1) {
        head.next = next;
    }

Input validation

I intentionally omitted input validation from these example implementations:
they won't work with position values outside the range \$[0, n]\$.
I leave that fun up to you.


Maximum value that satisfies a specific function I want to find the maximum value for a that doesn't change the output from another function. (_find_mean simply finds the location of the maximum element in a matrix)

This is how I am currently doing it and was wondering if there was a better way.

def _find_max_a(matrix1, matrix2):
    a = np.arange(0,10, 0.01)[::-1]
    for val in a:
        mat = matrix1 +  val * matrix2
        if _find_mean(matrix1) == _find_mean(mat):
            return val
        else:
            pass

arange can build a descending vector. Building an ascending vector and reversing it seems wasteful. Consider

a = np.arange(10, 0, -0.01)

matrix1 is never modified, but its _find_mean is recomputed on each iteration. Yet another waste. Compute it once.
Instead of linear search, you may want to consider a bisection of the search interval.
else: pass achieves nothing, and can be safely omitted.
_find_mean is a kinda strange name for a function which

  finds the location of the maximum element in a matrix


Simple maze game with four rooms I'm starting to learn Lua and in one part of the book I need to convert a "Maze Game" that uses goto to another one that doesn't use goto.

The maze is the simplest ever made, starting from room #1:

---------
| 1 | 2 |
---------
| 3 | 4 |
---------

This is the original version using goto from the book:

goto room1 -- initial room
::room1:: do
  local move = io.read()
  if move == "south" then goto room3
  elseif move == "east" then goto room2
  else
    print("invalid move")
    goto room1 -- stay in the same room
  end
end

::room2:: do
  local move = io.read()
  if move == "south" then goto room4
  elseif move == "west" then goto room1
  else
    print("invalid move")
    goto room2
  end
end

::room3:: do
  local move = io.read()
  if move == "north" then goto room1
  elseif move == "east" then goto room4
  else
    print("invalid move")
    goto room3
  end
end

::room4:: do
  print("Congratulations, you won!")
end

And this is my attempt:

local rooms = {}
rooms[1] = {
  south=3,
  east=2
}
rooms[2] = {
  south=4,
  west=1
}
rooms[3] = {
  north=1,
  east=4
}
currentRoom = 1
repeat
  local move = io.read()
  local room = rooms[currentRoom][move]
  if room == nil then
    room = currentRoom
    print("Invalid move")
  elseif room == 4 then
    print("Congratulations, you won!")
  end
  currentRoom = room or currentRoom
until currentRoom == 4

The above code is working but I was wondering if there is something I need to change to improve it somehow.

I think overall is good, some bits can be improved but it depends on taste. Take the following comments as suggestions.

As rooms is an array, can be defined in a shorter way as:

local rooms = {
   { south=3, east=2 },
   { south=4, west=1 },
   { north=1, east=4 },
}

Variable currentRoom can be localized:

local currentRoom = 1

In Lua can compare a var to nil or ask not var:

if not room then
   ...
end

Branches are exclusive, use of elseif can be skipped:

if not room then
   room = currentRoom
   print("Invalid move")
end
if room == 4 then
   print("Congratulations, you won!")
end

EDIT:

I would rearrange the code at the bottom as:

-- Update room state.
room = rooms[currentRoom][move]
if room then
  currentRoom = room
else
  print("Invalid move")
end

-- Reached final state?
if currentRoom == 4 then
  print("Congratulations, you won!")
end

that avoids the currentRoom = room or currentRoom statement (currentRoom only gets updated is room is not nil).


Single-instance WPF application This is the code I implemented so far to create a single instance WPF application:

#region Using Directives
using System;
using System.Globalization;
using System.Reflection;
using System.Threading;
using System.Windows;
using System.Windows.Interop;
#endregion

namespace MyWPF
{
    public partial class MainApplication : Application, IDisposable
    {
        #region Members
        private Int32 m_Message;
        private Mutex m_Mutex;
        #endregion

        #region Methods: Functions
        private IntPtr HandleMessages(IntPtr handle, Int32 message, IntPtr wParameter, IntPtr lParameter, ref Boolean handled)
        {
            if (message == m_Message)
            {
                if (MainWindow.WindowState == WindowState.Minimized)
                    MainWindow.WindowState = WindowState.Normal;

                Boolean topmost = MainWindow.Topmost;

                MainWindow.Topmost = true;
                MainWindow.Topmost = topmost;
            }

            return IntPtr.Zero;
        }

        private void Dispose(Boolean disposing)
        {
            if (disposing && (m_Mutex != null))
            {
                m_Mutex.ReleaseMutex();
                m_Mutex.Close();
                m_Mutex = null;
            }
        }

        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }
        #endregion

        #region Methods: Overrides
        protected override void OnStartup(StartupEventArgs e)
        {
            Assembly assembly = Assembly.GetExecutingAssembly();
            Boolean mutexCreated;
            String mutexName = String.Format(CultureInfo.InvariantCulture, "Local\\{{{0}}}{{{1}}}", assembly.GetType().GUID, assembly.GetName().Name);

            m_Mutex = new Mutex(true, mutexName, out mutexCreated);
            m_Message = NativeMethods.RegisterWindowMessage(mutexName);

            if (!mutexCreated)
            {
                m_Mutex = null;

                NativeMethods.PostMessage(NativeMethods.HWND_BROADCAST, m_Message, IntPtr.Zero, IntPtr.Zero);

                Current.Shutdown();

                return;
            }

            base.OnStartup(e);

            MainWindow window = new MainWindow();
            MainWindow = window;
            window.Show();

            HwndSource.FromHwnd((new WindowInteropHelper(window)).Handle).AddHook(new HwndSourceHook(HandleMessages));
        }

        protected override void OnExit(ExitEventArgs e)
        {
            Dispose();
            base.OnExit(e);
        }
        #endregion
    }
}

Everything works perfectly... but I have some doubts about it and I would like to receive your suggestions about how my approach could be improved.

I was asked by Code Analysis to implement IDisposable interface because I was using IDisposable members (the Mutex). Is my Dispose() implementation good enough? Should I avoid it because it's never going to be called?
It's better to use m_Mutex = new Mutex(true, mutexName, out mutexCreated); and check for the result or to use m_Mutex = new Mutex(false, mutexName); and then check for m_Mutex.WaitOne(TimeSpan.Zero, false); ? In case of multithreading I mean...
RegisterWindowMessage API call should return UInt32... but HwndSourceHook is only accepting Int32 as message value... should I be worried about unexpected behaviors (like a result bigger than Int32.MaxValue)?
In OnStartup override... should I execute base.OnStartup(e); even if another instance is already running and I'm going to shutdown the application?
Is there a better way to bring the existing instance to the top that doesn't need to set Topmost value? Maybe Activate()?
Can you see any flaw in my approach? Something concerning multithreading, bad exceptions handling and something like that? For example... what happens if my application crashes between OnStartup and OnExit?

This might be against the spirit of Code Review, but you don't need to write your own single instance manager for WPF! Microsoft has already written code to accomplish this, but it has been poorly advertised.

Microsoft's single instance manager is extremely comprehensive, and I have yet to find any issues with it. (And if you don't want to use it, it can at least be a good reference for your own implementation.)

How to manage single instances in WPF

Step 1: Add the System.Runtime.Remoting reference to your project.

Step 2: Add this single instance class to your project.

Step 3: Implement the ISingleInstanceApp interface in your main application class in App.xaml.cs (this interface is provided by the SingleInstance.cs file).

For example:

public partial class App : Application, ISingleInstanceApp

Step 4: Change your startup object by going to Project --> <projectname> Properties in Visual Studio, clicking the Application tab. Locate Startup object: and select the <projectname>.App option.

Step 5: Define a Main method in App.xaml.cs, and give it a unique string in the Unique variable.

// TODO: Make this unique!
private const string Unique = "Change this to something that uniquely identifies your program.";

[STAThread]
public static void Main()
{
    if (SingleInstance<App>.InitializeAsFirstInstance(Unique))
    {
        var application = new App();
        application.InitializeComponent();
        application.Run();

        // Allow single instance code to perform cleanup operations
        SingleInstance<App>.Cleanup();
    }
}

#region ISingleInstanceApp Members
public bool SignalExternalCommandLineArgs(IList<string> args)
{
    // Handle command line arguments of second instance
    return true;
}
#endregion

Step 6: Right-click on App.xaml in the Solution Explorer, select Properties, and change the Build Action to Page.

Single Instance techniques

Do nothing when new instance is opened

If you don't want anything to happen when a single instance is launched, you don't need to modify the sample code above.

Activate original window when new instance is opened

Rather than doing nothing when a user attempts to open a second instance of the program, it's often beneficial to Activate the original window and change its window state to WindowState.Normal, which can provide an improved user experience if the original window was minimized.

To activate and make the window visible, modify the SignalExternalCommandLineArgs method as shown . (Note that MainWindow is a property of the App class, not to be confused with an arbitrarily-named MainWindow.xaml.cs class.)

public bool SignalExternalCommandLineArgs(IList<string> args)
{
    // Bring window to foreground
    if (this.MainWindow.WindowState == WindowState.Minimized)
    {
        this.MainWindow.WindowState = WindowState.Normal;
    }

    this.MainWindow.Activate();

    return true;
}


Creating a complex hierarchy for "Player Effects" I'm coding a plugin for a game that has a Player class which has a boolean attribute frozen, which decides if the player is frozen or not. There are more "effects" like frozen, but I'm using it as an example, since solving one should solve them all.

The problem is, this frozen attribute only supports True/False, and if a player attacks an other player, freezing him for 5 seconds, and then someone else freezes the player permanently, the permanent freeze will go off after the other person's 5 second freeze ends (due to delay(5, setattr, (player, 'freeze', False)). This is why I need to implement a system that manages all the freezes applied to a player, and then unfreezes the player only when all of the freezes are gone.

Here's my original take on it, which I think is messy and I don't like the fact that I had to add the _effects dictionary to the Player class, when it's only used by the effect objects. I also have to rely on external _name attribute to identify the effect instances, and thus have to pass in an additional name parameter for the PlayerEffect decorator.

from ticks import delay, cancel_delay  # For delayed function calls
import game.core  # For the game's `Player` class

class PlayerEffect(object):

    def __init__(self, name, on_f=None, off_f=None, effect_cls=None):

        if effect_cls is None:
            class effect_cls(_PlayerEffect):
                pass

        effect_cls.name = name
        effect_cls.on_f = on_f
        effect_cls.off_f = off_f

        self._name = name
        self._on_f = on_f
        self._off_f = off_f
        self._effect_cls = effect_cls

    def on(self, on_f):
        return type(self)(self._name, on_f, self._off_f, self._effect_cls)

    __call__ = on

    def off(self, off_f):
        return type(self)(self._name, self._on_f, off_f, self._effect_cls)

    def __get__(self, instance, owner):
        return self._effect_cls(instance)

class _PlayerEffect(object):

    name = None
    on_f = None
    off_f = None

    def __init__(self, player):
        self._player = player

    def __call__(self, duration=-1):
        self._player._effects[self.name].append(self)
        if duration > 0:
            self._delay = delay(duration, self._disable)
        self.on_f(self._player)

    def _disable(self):
        self._player._effects[self.name].remove(self)
        if not self._player._effects[self.name]:
            self.off_f(self._player)

    def cancel(self):
        if self._delay is not None:
            cancel_delay(self._delay)
            self._delay = None
        self._disable()

class Player(game.core.Player):

    def __init__(self, index):
        super().__init__(index)
        self._effects = collections.defaultdict(list)

    @PlayerEffect('freeze')
    def freeze(self):
        self.frozen = True

    @freeze.off
    def freeze(self):
        self.frozen = False

The reason I need to have the _name is so that I can do stuff like this:

class Player(game.core.Player):

    def __init__(self, index):
        super().__init__(index)
        self._effects = collections.defaultdict(list)

    @PlayerEffect('fly')
    def fly(self):
        self.move_state = MoveStates.FLY

    @fly.off
    def fly(self):
        if self._effects['hover']:  # _name used here
            self.move_state = MoveStates.HOVER
        else:
            self.move_state = MoveStates.NORMAL

    @PlayerEffect('hover')
    def hover(self):
        self.move_state = MoveStates.HOVER

    @hover.off
    def hover(self):
        if self._effects['fly']:  # And here
            self.move_state = MoveStates.FLY
        else:
            self.move_state = MoveStates.NORMAL

I'd like to suggest a completely different implementation strategy for the, er, effects of the effects. You have a thing with mutable state (Player) and operations that change that state (self.move_state = ...) based on timers. You've had to add a special system to make the timers coordinate with each other, and if you ever make a mistake in the ”off” methods you will have a broken effect that sticks around.

Instead, make the state of the player a function of the effects. Here's a simple implementation to illustrate (note: may be Python 2-ish because I'm not familiar with 3; also untested):

def apply_effects(effects, attribute, base_value):
    value = base_value
    for effect in effects:
        value = effect.modify(attribute, value)
    return value

class Player(game.core.Player):
    def get_frozen(self):
        return apply_effects(self._effects, 'frozen', False)

    def get_move_state(self):
        return apply_effects(self._effects, 'move_state', MoveStates.NORMAL)

class Effect(object):
    """Base class for effects."""
    def modify(self, attribute, value):
        return value

class FrozenEffect(Effect):
    def modify(self, attribute, value):
        if attribute == 'frozen':
            return True
        else:
            super(FrozenEffect, self).modify(attribute, value)

class HoverEffect(Effect):
    def modify(self, attribute, value):
        if attribute == 'move_state' && value != MoveStates.FLY:
            return MoveStates.HOVER
        else:
            super(HoverEffect, self).modify(attribute, value)

class FlyEffect(Effect):
    def modify(self, attribute, value):
        if attribute == 'move_state':
            return MoveStates.FLY
        else:
            super(FlyEffect, self).modify(attribute, value)

(Note: If all effects are this simple, then they could be just functions instead of classes. Making them classes allows adding more operations, for example ”get name and icon” to show the player what effects apply.)

Then you would still manage the lifetime of the effects much like you are now, but the effects do not need to know each others' names. The only special rule here is that the FLY move state takes priority over the HOVER move state, which is presumably a basic rule about that enumerated type. (An alternative implementation would be to define a sort order for effects, sorting the list of effects so that the fly effect is always applied after the hover effect, or divide effects into groups that are applied in a specific order.)

This architecture also allows you to easily implement additive or multiplicative effects that stack with each other (e.g. speed/damage boosts).

If you need to actually have something happen when an effect ends, then save the computed values of the attributes, and when an effect ends compute the new attributes and do the relevant thing if they're different. This means that the happening is completely dependent on the state of the player and not what effects just happened. (For example, suppose two hover effects wear off at exactly the same moment: you don't want to start a ”drowned because fell in water” event twice, only once. This strategy can't make that mistake.)


Leetcode #91 - Number of ways to decode a string I'm working on problem 91 on Leetcode.com called Decode Ways and I've successfully managed to get a working recursive solution but it results in Time Limited Exceeded (TLE). I'm new to utilizing memoization and I've been unable to discover how to properly memoize my results at each recursive step and check if I've already solve the sub-problem.

Prompt:

  A message containing letters from A-Z is being encoded to numbers using the following mapping:

  'A' -> 1   'B' -> 2   ...   'Z' -> 26

  Given a non-empty string containing only digits, determine the total
  number of ways to decode it.

Working Code (TLE):

public int NumDecodings(string s)
{
    if (s == null || s.Length == 0) return 0;
    // int[] dp = new int[s.Length + 1];
    // for (int i = 0; i < dp.Length; ++i) dp[i] = -1;
    // dp[0] = 1;
    // dp[1] = Decode(s, 0, 1, dp) + Decode(s, 0, 2, dp);
    int jump1 = Decode(s, 0, 1);
    int jump2 = Decode(s, 0, 2);
    return jump1 + jump2;
    // return dp[1];
}

public int Decode(string s, int i, int len)
{
    if (i + len - 1 >= s.Length)
        return 0;

    // if (dp[i] > -1)
    //     return dp[i];

    int sub = Convert.ToInt32(s.Substring(i, len));
    if (sub > 26 || sub < 1 || s.Substring(i, len)[0] == '0')
        return 0;
    else if (i + len - 1 == s.Length - 1 && sub < 27 && sub > 0)
        return 1;

    int jump1 = i + len - 1 + 1 < s.Length ? Decode(s, i + len, 1) : 0;
    int jump2 = i + len - 1 + 2 < s.Length ? Decode(s, i + len, 2) : 0;
    // dp[i] = jump1 + jump2;

    return jump1 + jump2;
    // return dp[i];
}

Memoization

Looking into memoization is a good idea - in this case it allows you to achieve \$O(n)\$ runtime performance instead of \$O(2^n)\$. One way to add memoization is to create a wrapper method that takes the same arguments as Decode. This method first looks up the given arguments in a dictionary, and if they're present, it can return the memoized result. If not, it will call Decode and store its result in the dictionary before returning it.

However, I would recommend removing NumDecodings and rewriting Decode so it only takes a single string parameter. That will simplify memoization and make it a little more efficient. For example, "412123" and "512123" both have the same suffix ("12123"), but because the 'sub-calls' ("412123", 1, 1) and ("512123", 1, 1) use different arguments (and thus different memoization keys) you won't benefit from memoization across those different inputs.

Other notes

Instead of passing the original input string and an offset and length, consider passing 'the rest of the string' instead. Decode can then call Decode(s.Substring(1)) if the leading digit is a valid unit, and Decode(s.Substring(2)) if the leading 2 digits are a valid unit.
There are various things in the current code that can be simplified:

if (i + len - 1 >= s.Length) to if (i + len > s.Length)
s.Substring(i, len)[0] == '0' to s[i] == '0'
else if (i + len - 1 == s.Length - 1 to else if (i + len == s.Length
jump1 = i + len - 1 + 1 to jump1 = i + len
jump2 = i + len - 1 + 2 to jump2 = i + len + 1

Moving s.Substring(i, len)[0] == '0' to an else if branch allows you to remove the sub < 27 && sub > 0 checks below.
Disabling code during development is normal, but it's a good idea to clean things up from time to time.
I would rename Decode to something more descriptive like ValidDecodingsCount - Decode implies that it actually decodes the given input, which is not the case. jump1 and jump2 aren't very descriptive names either, but it's a bit difficult to come up with better names. Maybe rest1Combinations and rest2Combinations?


Checking for uniqueness within a string I saw this interview question in the book Cracking the Coding Interview:

  Implement an algorithm to determine if a string has all unique characters

The authors solution uses bit-shifting for a pretty efficient solution.

I wanted to give it a shot to discover some new things and practice some. I'd like for this to be a discussion of how I can improve my programming and also, what the correct answer may be. Did I leave out any edge-cases? Is there a more efficient way of doing this?

#include <iostream>
#include <string>
#include <map>

using namespace std;

// Uses an iterator to display the map.
void displayMap(map<char, int> displayCharacters)
{
    for (map<char, int>::iterator itr = displayCharacters.begin(), end = displayCharacters.end(); itr != end; ++itr)
    {
        cout << itr->first << " --> " << itr->second << endl;
    }
}

// Checks for uniqueness for a given string and returns a boolean.
bool isUnique(int stringLength, string alphabet, map<char, int> uniqueCharacters)
{
    for (int i = 0; i < stringLength; i++)
    {
        if (uniqueCharacters[alphabet[i]] == 1)
        {
            return false;
        }

        uniqueCharacters[alphabet[i]]++;
    }

    return true;
}

// Maps individual characters found in stringRead and sets a value to them.
// Returns the mapped map.
map<char, int> readString(string stringRead, map<char, int> uniqueCharacters)
{
    for (int i = 0, strLen = stringRead.length(); i < strLen; i++)
    {
        uniqueCharacters[stringRead[i]] = 0;
    }

    return uniqueCharacters;
}

int main()
{
    // Create a map to store a key character and value int.
    // Create a string to hold a variable string to test.
    map<char, int> uniqueCharacters;
    string testString = "aaabbbccc";
    int stringLength = testString.length();

    // uniqueCharacters is mapped to the above string and the displayed using displayMap.
    uniqueCharacters = readString(testString, uniqueCharacters);
    displayMap(uniqueCharacters);

    if (isUnique(stringLength, testString, uniqueCharacters))
    {
        cout << "Unique string!\n";
    }
    else
    {
        cout << "Not unique!\n";
    }

    cin.ignore();
    cin.get();
}

This solution is inefficient:

There is no point counting the characters in the entire string: once you found a duplicate, you can stop counting, because you know the result is false

This also implies that the map of counts is wasted space

Iterating over the alphabet to check character counts is inefficient: it would be better to iterate over the map values

Instead of a map, it would be more efficient to use a set.

Instead of a set, you could also consider a boolean array of the size of the alphabet, values initialized to false,
and use that to track characters seen so far.
The disadvantage of this approach over a set is that it might use more space.

You are also violating some good practices:

using namespace std is considered bad practice
readString is poorly named: it doesn't "read a string", it initializes a map of character counts to all zero values
No need to pass stringLength together with testString. You can derive that value from testString


Reading integers from a file and counting the length of a descending subsequence I had a school project to solve, which I did. The basic focus of the project was to create the simplest code possible to solve it. This topic was created just to ensure if I wrote the code correctly.

The problem is the following:

  We have declared a dynamic array which will take numeric values from a txt file. We
  want to read the array backwards and to make a simple check.
  Read every 'i' element in the array and check if the value of the array is greater than > the last one.If it is the value saved in the int called temp_max which will be the > value of the array in the i position. The program will continue to check every number
  and if a value in the array is greater than temp_max we will overwrite the value of the temp_max to the value of the array in that position.

#include <stdio.h>
#include <stdlib.h>

int main() {

    FILE *input, *output;
    int *children;
    int size, i;

    input = fopen("xxx.in", "r");

    if(input == NULL)
    return 1;

    fscanf(input,"%d",&size);

    if(size < 1 || size > 1000000)
    return 1;

    children = malloc(size * sizeof(int));

    if (children==NULL)
    return 1;

    for ( i = 0; i < size; i++ )
    {
        fscanf(input, "%d",&children[i]);
    }
    fclose(input);

    int total = 0;
    int temp_max = 0;
    for(i = size - 1; i >= 0; i--)
    {
        if(children[i] > temp_max) {
            temp_max = children[i];
            total++;
        }
    }

    free(children);

    output = fopen("xxx.out", "w+");
        fprintf(output, "%d\n", total );
        fclose(output);

    //This is the end, my dear friend!
    exit(0);
}

The int size was declared in one step above and is the size of the dynamic array.
The int total calculate how many times a number is greater than another. Default value is zero cause, I believe that should be what is your opinion.
The array hasn't any zero values or negative values at all.

The text file has the following format:

Example (xxx.in) 1:

5
9 6 5 4 2

Example (xxx.in) 2:

6
8959 5594 595 2 949 9

int main() returns an int so instead of exit(0); write return 0; at the end of main, it is more natural. exit is more something you would use to panically exit a program.

When you write the file xxx.out, you don't check if opening the file succeeded, it always good to check return values of all runtime functions like you do when you read the other file.

Initialize all variables when you declare them to avoid any surprises. You have in some cases declared the variables where you use them, that is good. You should do that with all variable declarations instead of at top of main.

Check return value of fscanf() to see if it correctly got a number. I personally prefer fgets/sscanf when reading from file/stdin but that is me.

Maybe you could separate the functionality into some smaller functions e.g. readFile, writeFile and calculateTotal. It is a good habit to learn because programs have a tendency to grow with time, although for this particular case it may be overkill.

Another good habit is to write "why" comments, but I guess in this particular case it is quite self-evident what it does, although maybe a mention of the file format expected in the header of the program would be helpful for another programmer. :)

You could add some error messages to stderr when something is wrong instead of silently quitting, it makes troubleshooting easier e.g.

fprintf(stderr, "invalid size %d", size);


Is there a better way how to force file download than using this simple PHP script? I've written this simple script to force download some files (jpg, mp3, usually those which are loaded in browser by default).
I was wondering whether there's any way this could be improved upon, which means:

making it more secure
making it use less cpu (filesize(), fopen(), fpassthru() are the usual suspects here)
making it simpler maybe

Here it goes:

<?php

if (!isset($_GET['file'])){
   die('no file requested');
}

else{

   if (substr($_GET['file'], 0, 1) == '.'){
      die('trying to leave this directory? :)');
   }

   $path = './'.$_GET['file'];
   if (file_exists($path) && is_readable($path)){
    $size = filesize($path);
    header('Content-Type: application/octet-stream');
    header('Content-Length: '.$size);
    header('Content-Disposition: attachment; filename='.$_GET['file']);
    header('Content-Transfer-Encoding: binary');
    $file = fopen($path, 'rb');
    fpassthru($file);
    exit;
   }

}

?>

Usage: /?file=sample.jpg

Always use === in your comparisons.

if (substr($_GET['file'], 0, 1) == '.'){
  die('trying to leave this directory? :)');
}

Should be:

if (substr($_GET['file'], 0, 1) === '.'){
  die('trying to leave this directory? :)');
}

Also look into other ways to sanitize $_GET['file'] properly at the moment I could pass in something like /../ and I could access other files.

From the docs you can save on a fopen() call by using readfile() instead see fpassthru()

A better way to use variables in a string is to use curly syntax so this line

header('Content-Disposition: attachment; filename='.$_GET['file']);

Would be better as

header("Content-Disposition: attachment; filename={$_GET['file']}");

I think that covers the three bullet points :)


Excel VBA speed up code to hide rows/delete color I am using following code to delete color and hide rows that include several criteria before exporting sheets to pdf's. Is there any way to speed up this process as it is taking quite a lot of time to process. Especially in situations when I have several sheets in one workbook and to apply this on each sheet = "printed page".

Sub Color()
    Dim myRange As Range
    Dim cell As Range
    Application.ScreenUpdating = False
    Set myRange = ThisWorkbook.Sheets("Print version").Range("Print_Area")
        For Each cell In myRange
        myRange.Interior.ColorIndex = 0
        If cell.HasFormula = True And cell.Value = "" And cell.EntireRow.Hidden = False Then Rows(cell.Row).EntireRow.Hidden = True
    Next
    Application.ScreenUpdating = True
End Sub

Your test for hidden rows suggests you may already have hidden rows.

Nested IF statements could reduce the number of tests to be done.

As you are removing color from all cells, do it before the loop:

Sub Color()
    Dim myRange As Range
    Dim cell As Range
    Application.ScreenUpdating = False
    Set myRange = ThisWorkbook.Sheets("Print version").Range("Print_Area")
        myRange.Interior.ColorIndex = 0
        For Each cell In myRange
            if Not cell.EntireRow.Hidden then
                '  once hidden, no more cell values will be tested on the row
                If cell.HasFormula = True And cell.Value = "" then
                    Rows(cell.Row).EntireRow.Hidden = True
                endif
            endif
        Next
    Application.ScreenUpdating = True
End Sub


Representing vehicles and their components I am working on a project that needs to represent different kinds of vehicles, components of these vehicles and actual parts of the components.

So far I have this code:

class Vehicle
{
public:
    /* Public interface */

    //ctor
    Vehicle(int id, int year, int mileage, int weight, int length, int width, int height, int wheelbase,
        const QString& date, const QString& type, const QString& maker, const QString& model, const QString& color, const QString& plate);

    //copy/move
    Vehicle(const Vehicle&) = delete;
    Vehicle(Vehicle&&) = delete;
    Vehicle& operator=(const Vehicle&) = delete;
    Vehicle& operator=(Vehicle&&) = delete;

    //dtor
    virtual ~Vehicle();

    //getters
    inline int getId() const { return m_id; }
    inline int getYear() const { return m_year; }
    inline int getMileage() const { return m_mileage; }
    inline int getWeight() const { return m_weight; }
    inline int getLength() const { return m_length; }
    inline int getWidth() const { return m_width; }
    inline int getHeight() const { return m_height; }
    inline int getWheelbase() const { return m_wheelbase; }

    inline const QString& getDateReceived() const { return m_date_received; }
    inline const QString& getType() const { return m_type; }
    inline const QString& getMaker() const { return m_maker; }
    inline const QString& getModel() const { return m_model; }
    inline const QString& getColor() const { return m_color; }
    inline const QString& getPlate() const { return m_plate; }

    inline const QList<QImage*>& getImages() const { return m_images; }
    inline const QImage& getThumbnail() const { return m_thumbnail; }

protected:
    /* Protected methods */

    //adds another component to this vehicle
    inline void addComponent(Component* c)
    {
        m_components << c;
    }

private:
    /* Private data members */

    QList<Component*> m_components;

    int m_id;
    int m_year;
    int m_mileage;
    int m_weight;
    int m_length;
    int m_width;
    int m_height;
    int m_wheelbase;

    QString m_date_received;
    QString m_type;
    QString m_maker;
    QString m_model;
    QString m_color;
    QString m_plate;

    QList<QImage*> m_images;
    QImage m_thumbnail;
};

class Car : public Vehicle
{
public:
    /* Public interface */

    //ctor
    Car();

    //copy/move
    Car(const Car&) = delete;
    Car(Car&&) = delete;
    Car& operator=(const Car&) = delete;
    Car& operator=(Car&&) = delete;

    //dtor
    virtual ~Car();

private:
    /* Private data members */

    Body* m_body;
    Brake* m_brake;
    Direction* m_direction;
    Engine* m_engine;
};

I also have Moto and Bike classes publicly inheriting from Vehicle, each of them having their own components. Then, each component derives from Component and has several parts (deriving from Part, i.e. a Body has Wings, Capote, etc). Both Component and Part inherit from AbstractComponent.

First off, is this bad design? How can it be improved?

Secondly, I have a question about creating the specific vehicle objects. Should Car's constructor require the same parameters as Vehicle plus others? How about its components? Should I pass to the constructor pointers to already created Components, or should the Components be created in Car's constructor?

The architecture seems fine to me the way it is. It's nice that you have used a little bit of componentization instead of doing some absurd inheritance chain in Car. Maybe someone else will have more comments on the design, but if you are not having problems with it, my opinion is to leave it as it stands.

A few other details I would like to comment on:

There doesn't seem to be any method that changes a Vehicle after an instance is constructed. This is a good design. You should favor immutability whenever practical. Since this is the case, you should further enforce that the vehicles are not mutated by making its member variables const. Then you only allow setting them once in the constructor. E.g.:

const int m_id;
const int m_year;
const int m_mileage;
const int m_weight;
const int m_length;
const int m_width;
const int m_height;
const int m_wheelbase;
...

You probably won't be able to make the QLists const though. But that's not a big deal. Mark with const what you can.
Vehicle's constructor has a lot of parameters. It might be confusing to call it, since many parameters have the same type. One way to consolidate the large number of parameters would be having a sort of VehicleDesc helper structure grouping all the parameters. But that might or might not be a better solution, it depends. It also doesn't simplify setting up a Vehicle. Another possibility might be constructing from a stream or file (AKA serialize/de-serialize).
When you already delete the copy constructor and assignment operator you don't have to also delete the move ones. Remember that move operator and constructor are not added by default to all classes by the compiler, unlike the copy/assign ones. Quoting this very good StackOverflow answer:

  Even if you want your class to not be copyable nor movable, it is
  better to just delete the copy members and leave the move members
  undeclared (meaning they won't exist). If you're ever reviewing code
  (including your own), and see deleted move members, they are almost
  certainly incorrect, or at the very best superfluous and confusing.

You don't need to add the inline keyword when declaring a member method directly inside the class body in the header file. Such methods are already implicitly (to the compiler) and clearly (to the reader) inline.
Seconding for a comment in your post, don't write self evident comments like //ctor or //copy/move. That's just visual pollution, plus, if you shuffle class sections around and forget to move the comments with it, it will create discrepancies.
About your question of Car's constructor replicating the parameters of Vehicle, if you have access to C++11, the you'll probably want to use constructor inheritance. Otherwise, I'm afraid you will have to replicate the constructor's parameter list and just forward the data to Vehicle.
Finally, the ownership of pointers in your code worries me a little. Car has raw pointers to its components, which I assume are also replicated in the components list of Vehicle. There is a certain potencial for memory bugs there. You are probably very familiar with the code and knows exactly when things get allocated and deleted, but don't expect the next guy you pass this project on to be that familiar with it (or even yourself might forget about it in a few months time!). People maintaining your code are much more likely to introduce bugs than you are. For this reason, mainly, I would advocate for the use of smart pointers. Car should probably hold shared_ptrs to its components, so that Vehicle can store weak_ptrs in its component list.


Fill missing values based on group I am trying to fill values based on group, in my case id. I would like to fill the missing values according to the available date info for each id.

   id  date
1   1 23-04
2   1 23-04
3   1  <NA>
4   1  <NA>
5   2 24-04
6   2  <NA>
7   2  <NA>
8   2  <NA>
9   3 23-04
10  3  <NA>
11  3  <NA>
12  3  <NA>
13  4  <NA>
14  4  <NA>
15  4  <NA>
16  4  <NA>

What I need is:

    id  date
1   1 23-04
2   1 23-04
3   1 23-04
4   1 23-04
5   2 24-04
6   2 24-04
7   2 24-04
8   2 24-04
9   3 23-04
10  3 23-04
11  3 23-04
12  3 23-04
13  4  <NA>
14  4  <NA>
15  4  <NA>
16  4  <NA>

I figured out a loop, but I would like to avoid it because my data has 23 millions rows:

for(i in 2:nrow(dta)){
  if(dta$id[i-1] == dta$id[i])
  {
    dta$date[i] = dta$date[i-1]
  }
}

I cannot figure out how to translate this into dplyr syntax:

dta = structure(list(id = structure(c(1L, 1L, 1L, 1L, 2L, 2L, 2L, 2L,
3L, 3L, 3L, 3L, 4L, 4L, 4L, 4L), .Label = c("1", "2", "3", "4"
), class = "factor"), date = structure(c(1L, 1L, 1L, 1L, 2L,
2L, 2L, 2L, 1L, 1L, 1L, 1L, NA, NA, NA, NA), .Label = c("23-04",
"24-04"), class = "factor")), .Names = c("id", "date"), row.names = c(NA,
-16L), class = "data.frame")

In R this is usually solved using the na.locf (Last Observation Carried Forward) function from the zoo package.

See also here:

Fill in NA based on the last non-NA value for each group in R
Using dplyr window-functions to make trailing values

# test data
x <- read.table(text="id;date
1;23-04
1;23-04
1;NA
1;NA
2;24-04
2;NA
2;NA
2;NA
3;23-04
3;NA
3;NA
3;NA
4;NA
4;NA
4;NA
4;NA", header=TRUE, sep=";")

library("dplyr")
library("zoo")

x %>% group_by(id) %>% transmute(date=na.locf(date, na.rm=FALSE))
Source: local data frame [16 x 2]
Groups: id

   id  date
1   1 23-04
2   1 23-04
3   1 23-04
4   1 23-04
5   2 24-04
6   2 24-04
7   2 24-04
8   2 24-04
9   3 23-04
10  3 23-04
11  3 23-04
12  3 23-04
13  4    NA
14  4    NA
15  4    NA
16  4    NA

Another option are rolling self-joins supported by the data.table package (see here).


Bills and coin denomination program I am currently in an intro C++ programming class. We have an assignment where we have to convert a dollar and cents amount, say $192.89, to bills and coins using the least amount of both. Below is the program I have written.

#include <iostream>

using std::cout;
using std::cin;
using std::endl;

int main()
{
    double TotalAmount;
    int DollarsConversion;
    int Change;
    int Dollars;
    int Hundreds = 0;
    int Fifties = 0;
    int Twenties = 0;
    int Tens = 0;
    int Fives = 0;
    int Ones = 0;
    int CentsConversion;
    int Cents;
    int Quarters = 0;
    int Dimes = 0;
    int Nickels = 0;
    int Pennies = 0;

    cout << "Please enter the amount to convert: $";
    cin >> TotalAmount;

    //Dollars Calculation.
    DollarsConversion = TotalAmount * 100;
    Hundreds = DollarsConversion / 10000;
    Change = DollarsConversion % 10000;
    Fifties = Change / 5000;
    Change %= 5000;
    Twenties = Change / 2000;
    Change %= 2000;
    Tens = Change / 1000;
    Change %= 1000;
    Fives = Change / 500;
    Change %= 500;
    Ones = Change / 100;
    Change %= 100;

    //Resets the Stack to calculate for the cents.
    Dollars = TotalAmount;
    CentsConversion = TotalAmount * 1000;
    Dollars *= 1000;
    Cents = CentsConversion - Dollars;

    //Cents Calculation.
    Quarters = Cents / 250;
    Change = Cents % 250;
    Dimes = Change / 100;
    Change %= 100;
    Nickels = Change / 50;
    Change %= 50;
    Pennies = Change / 10;
    Change %= 10;

    cout << "\nNumber of Hundred Dollar Bills: " << Hundreds << endl;
    cout << "Number of Fifty Dollar Bills: " << Fifties << endl;
    cout << "Number of Twenty Dollar Bills: " << Twenties << endl;
    cout << "Number of Ten Dollar Bills: " << Tens << endl;
    cout << "Number of Five Dollar Bills: " << Fives << endl;
    cout << "Number of One Dollar Bills: " << Ones << endl;

    cout << "\nNumber of Quarters: " << Quarters << endl;
    cout << "Number of Dimes: " << Dimes << endl;
    cout << "Number of Nickels: " << Nickels << endl;
    cout << "Number of Pennies: " << Pennies << endl;

    return 0;
}

You have a lot of copy-and-paste code, where the only things that vary are the variable name, denomination, and textual description.  You can generalize the logic.

#include <iostream>

struct Denomination {
    const int cents;
    const char *description;
};

const Denomination DENOMINATIONS[] = {
    { 10000, "Hundred-Dollar Bills" },
    {  5000, "Fifty-Dollar Bills" },
    {  2000, "Twenty-Dollar Bills" },
    {  1000, "Ten-Dollar Bills" },
    {   500, "Five-Dollar Bills" },
    {   100, "One-Dollar Bills" },
    {    25, "Quarters" },
    {    10, "Dimes" },
    {     5, "Nickels" },
    {     1, "Pennies" }
};

int main() {
    double totalAmount;
    std::cout << "Please enter the amount to convert: $";
    std::cin >> totalAmount;
    std::cout << '\n';

    int cents = 100 * totalAmount;
    for (int i = 0; i < sizeof(DENOMINATIONS) / sizeof(DENOMINATIONS[0]); ++i) {
        std::cout << "Number of " << DENOMINATIONS[i].description
                  << ": " << (cents / DENOMINATIONS[i].cents) << '\n';
        cents %= DENOMINATIONS[i].cents;
    }
}

Also note:

A somewhat common convention is to use this NamingConvention for types, this namingConvention for variable names, and this NAMING_CONVENTION for constants.
Your DollarsConversion variable is misnamed, in my opinion, as it actually stores the number of cents.