Turing machine schoolwork Our teacher doesn't give much feedback on our code itself. Was just looking for some feedback on how I could improve on this. This is the method that tests the string against the Turing machine that is built from a file.

The entire project is on GitHub.

public boolean run(String input) {
    String currentState = startState;
    ArrayList<Transition> pT;
    ArrayList<Character> chars = new ArrayList<>();
    int i = 0;

    for (char c : input.toCharArray()) {
        if (!inputAlphabet().contains(c)) {
            return false;
        } else {
            chars.add(c);
        }
    }
    tape.setCells(chars);
    tape.setHeadPosition(i);
    System.out.println(currentState);

    while (i != tape.getCells().size()) {
        pT = new ArrayList<>();
        for (Transition t : transitions) {
            if (t.getFromState().equals(currentState)) {
                pT.add(t);
            }
        }

        for (int j = 0; j < pT.size(); j++) {
            if (tape.getCells().get(i) == pT.get(j).getInputSymbol()) {
                if (pT.get(j).getWriteSymbol() != EMPTY_SYMBOL) {
                    ArrayList<Character> temp;
                    temp = tape.getCells();
                    temp.set(i, pT.get(j).getWriteSymbol());

                    tape.setCells(temp);
                }

                if (pT.get(j).getDirection().charAt(0) == DIRECTION_RIGHT) {
                    i++;
                    tape.setHeadPosition(i);
                } else if (pT.get(j).getDirection().charAt(0) == DIRECTION_LEFT) {
                    i--;
                    tape.setHeadPosition(i);
                }
                currentState = pT.get(j).getToState();
                System.out.println(currentState);
                j = pT.size();
            }
        }
        if (rejectStates.contains(currentState)) {
            return false;
        }
    }

    pT = new ArrayList<>();
    for (Transition t : transitions) {
        if (t.getFromState().equals(currentState)) {
            pT.add(t);
        }
    }

    for (int k = 0; k < pT.size(); k++) {
        if (pT.get(k).getInputSymbol() == EMPTY_SYMBOL) {
            currentState = pT.get(k).getToState();
            System.out.println(currentState);
            if (pT.get(k).getDirection().charAt(0) == DIRECTION_RIGHT) {
                i++;
                tape.setHeadPosition(i);
            } else if (pT.get(k).getDirection().charAt(0) == DIRECTION_LEFT) {
                i--;
                tape.setHeadPosition(i);
            }
            k = pT.size();
        }
    }

    return acceptStates.contains(currentState);
}

Some grist for the mill:

When variables will not be reassigned, it's helpful to declare them as final to reduce cognitive load on the reader.

When using the Collections API, it's preferable to declare interface types rather than implementations (List instead of ArrayList) unless you need functionality specific to the implementation type.

Declare variables as closely to where they're first used as is possible.

When you return in the if clause, you don't need an else clause.

Calling inputAlphabet().contains() once for every character in the input is very inefficient, especially given that inputAlphabet() is running a loop to create the list. Call inputAlphabet() once and keep a local variable to hold it. And have inputAlphabet() return a Set instead of a List, which gets you contains() at O(1) instead of O(n).

input.toCharArray() is easy to read, which makes it probably correct in this case, but be aware that it's marginally less efficient than using an indexed loop and calling input.charAt(i) because it creates a new char[].

You don't need to keep a Tape instance variable. Just create a new one in run(). It might be nice if the Tape constructor accepted a list of cells.

Probably not relevant, but your code is not thread-safe. It will break if multiple threads call run at the same time.

Don't reuse a variable (pT) for different things in the same method. It makes it hard for the reader to keep track of what it contains.

pT is a poor variable name. I think it means possibleTransitions? Variables should clearly indicate what they're referencing. Another example is t instead of transition.

The loop building the transitions could be extracted into a method.

It's traditional in looping constructs to check <=, instead of != to avoid a possible infinite loop if a bug puts you past the exact value you want to terminate on.

You can localize i and use a for loop, because you know is has to be tape.getCells().size() when you're done looping.

Your code might be easier to read with a guard clause if (.. != .. ) { continue; rather than nesting.

Rather than setting the value of j to break the loop, just use break. Then you can use an enhanced for loop rather than indexing on j.

Your temp dancing is not meaningful. Since Tape is returning an unsafe copy from getCells(), you could just do tape.getCells().set(i, ..). Even better would be changing the API of Tape to have the methods to access the cells (tape.getCellAt(int), tape.setCellAt(int, Character) instead of returning the array for you to mess around with. While you're at it, add a method like size() or length() to tape and get rid of getCells altogether. Let Tape hide the fact that it's using a List under the covers, and you can change the implementation later without breaking other code.

It's unclear why a Transition's direction and the current state are treated as Strings when they're only ever one character long.

It also be nice if Transition could be changed so you could do something like i = possibleTransition.applyDirection(i), and the Transition determines whether to add one, subtract one, or do nothing. Then that if .. else if .. block reduces to i = possibleTransition.applyDirection(i); tape.setHeadPosition(i);

Your second for loop with the k index has the same break; issue as the first. You can again use the enhanced for loop.

It's unclear what this loop is doing with Tape, since you're making changes that effectively vanish once the method exists. It looks like that code can just go. In fact, since getHeadPosition is never called in your codebase, I'm not sure why you're tracking it at all.

If you were to make all of these modifications, your code might look something like the code below. This is untested, so I might very well have broken something.

public boolean run(final String input) {
    final List<Character> chars = new ArrayList<>();
    final Set<Character> inputAlphabet = inputAlphabet();
    for (final char c : input.toCharArray()) {
        if (!inputAlphabet.contains(c)) {
            return false;
        }
        chars.add(c);
    }

    String currentState = startState;

    final Tape tape = new Tape(chars);
    for (int i = 0; i < tape.size(); i++) {

        for (final Transition transition : this.possibleTransitions(currentState)) {
            if (tape.getCellAt(i) != transition.getInputSymbol()) {
                continue;
            }

            if (transition.getWriteSymbol() != EMPTY_SYMBOL) {
                tape.setCellAt(i, transition.getWriteSymbol());
            }

            i = transition.applyDirection(i);
            currentState = transition.getToState();
            break;
        }

        if (this.rejectStates.contains(currentState)) {
            return false;
        }
    }

    for (final Transition transition : possibleTransitions(currentState)) {
        if (transition.getInputSymbol() == EMPTY_SYMBOL) {
            currentState = transition.getToState();
            break;
        }
    }
    return acceptStates.contains(currentState);
}

private List<Transition> possibleTransitions(final String state) {
    final List<Transition> possibleTransitions = new ArrayList<>();
    for (final Transition transition : transitions) {
        if (transition.getFromState().equals(state)) {
            possibleTransitions.add(transition);
        }
    }
    return possibleTransitions;
}


Sieve of Eratosthenes in JavaScript I found this method kinda tough to get for a beginner like me, but I tried to do my best, and here's what I came up with.

Is this good code regarding performance? Is there anything wrong with it?

var notPrime = [] ;
var prime = [] ;

var n = prompt("Enter n: ");

for(var i = 2 ; i < n ; i++ ){

    if(notPrime.indexOf(i) != -1){
        continue;
    }

    for(var j = i ; i <= j ; j++){
        if((i * j) < n){
            notPrime.push(i*j);
        } else {
            break;
        }
    }
    prime.push(i);
}

for(var f in prime){
    console.log(prime[f]);
}

Lets say that there is room for improvement. ;)

The prompt method returns a string, but you want a number, so you should parse the string:
var n = parseInt(prompt("Enter n: "), 10);

Using indexOf on an array is slow. Instead of putting all the non-primes in a bucket and rummaging through it, you should use an array containing boolean values where the index is the number and the values tells you if it's a prime or not.
Accessing an array by index is an O(1) operation, while searching for a value in an array is an O(n) operaton. As n grows, this code will get slower and slower the longer you let it run.
As you are pushing a lot of duplicate non-primes in the array, an array of boolean values will actually use about 70% less memory eventhough the primes also takes up space in it.

This loop is pretty pointless:
for(var j = i ; i <= j ; j++){

The condition will never be false. You should instead use the i * j < n condition to break out of the loop:
for(var j = i; i * j < n; j++){
  notPrime[i * j] = true;
}

Edit:
You are still using the array as a kind of collection. You should set all values to true at start, and then set all non-primes to false. This code (based on the algorithm here) is about five times faster:
var n = parseInt(prompt("Enter n: "), 10);
var i, j;
var prime = new Array(n);
for (i = 2; i < n ; i++) prime[i] = true;

for (i = 2; i * i < n ; i++) {
  if (prime[i]) {
    for (j = 0; i * i + i * j < n ; j++) {
      prime[i * i + i * j] = false;
    }
  }
}

var cnt = 0;
for (i = 2 ; i < n ; i++) {
  if (prime[i]){
    console.log(i);
  }
}


Cleanly passing in a large number of mutable parameters through a python class I want to create two classes, one for attributes and one for functional behaviors. The point of the attributes class was to hold all of the attributes along with property methods (setters/getters), and I wanted to split the classes up this way so that everything related to the attributes would not fill up the entire other class.
Due to the way python handles mutable default arguments, I have to handle the attributes using None for default arguments, but I have a very large number of parameters. This has caused the code to become unnecessarily long when passing arguments around on the initialization:
class Attributes:
    def __init__(self,
            arg1,
            arg2,
            arg3,
            arg4,
            arg5):

        self.arg1 = default_arg1 if arg1 is None else arg1
        self.arg2 = default_arg2 if arg2 is None else arg2
        self.arg3 = default_arg3 if arg3 is None else arg3
        self.arg4 = default_arg4 if arg4 is None else arg4
        self.arg5 = default_arg5 if arg5 is None else arg5

    # attribute methods like getters and setters

class Functionality(Attributes):
    def __init__(self,
            arg1 = None,
            arg2 = None,
            arg3 = None,
            arg4 = None,
            arg5 = None):

        super(Functionality, self).__init__(
            arg1,
            arg2,
            arg3,
            arg4,
            arg5
        )

    # Methods that give functionality to the class

I want to be able to use this class as follows:
example_obj = Functionality(
    arg1 = example_arg1,
    arg4 = example_arg4
)

Is there any cleaner way to do this? I want to add more attributes (more than 20 attributes instead of 5), but the above code involves writing basically the same thing way too many times.

I'm going to address your existing code in a manner that generally goes from most minimal to most significant changes. I'd strongly recommend actually using the solution at the very bottom of this post; the minimal changes are more useful:

From a historical (pre-dataclasses) perspective
For understanding how classes and inheritance work

but there's no reason to torture yourself hand-writing all of this when dataclasses will do the work for you.
Delegating to defaults of super class
When you're inheriting __init__ from another class, you typically don't want to reproduce their parameters explicitly; it creates too much confusion and too much interdependency (making more opportunities for code to get out of sync). There are two standard solutions here:
Option 1: Explicitly delegate via **kwargs
When Functionality doesn't make direct use of any parameter, don't accept it by name. Just accept **kwargs (when you're talking about this many defaulted parameters, no one should be passing arguments positionally anyway; it's a nightmare for readability/maintainability) and pass it along. So Functionality would look like:
class Functionality(Attributes):
    def __init__(self, **kwargs):
        # Other stuff required for initialization
        super().__init__(**kwargs)  # Python 3.x doesn't require you to pass args to super in most cases
        # Other stuff required for initialization

    # Methods that give functionality to the class

Option 2: Inherit __init__ implicitly
In this case, it might be even simpler though; the __init__ of Functionality doesn't do anything beyond delegate to Attributes. If your real code is the same (nothing but a super().__init__ call), you'd just omit the definition of __init__ on Functionality and let it inherit Attributes's __init__ directly.
class Functionality(Attributes):
    # No __init__ defined at all; uses Attributes.__init__ automatically

    # Methods that give functionality to the class

Either way, you're no longer using separate defaults for each class.
Note: If the parent class must not accept default arguments, but the child class should, see the end of this answer for Preferred solution (especially if the child must not have defaults, and the parent class should): dataclasses everywhere (not described here since it relies on techniques for solving your mutable defaults problem, which I haven't gotten to yet).

Avoid problems with mutable defaults
There are two simple solutions for avoiding the problems with mutable defaults.
Option 1: Deep copy unconditionally
For the defaulted class itself, go ahead and use mutable arguments as defaults, but the safe way, making deep copies. This is often safer even when not passed as defaults, since without copying, you'd be aliasing values from the caller, and changes made by either you or the caller would affect the other.
Adding explicit mutable defaults, you end up with:
from copy import deepcopy

class Attributes:
    def __init__(self, arg1=[], arg2={}, arg3=set(), arg4=MyMutable(), arg5=OtherMutable()):
        self.arg1 = deepcopy(arg1)
        self.arg2 = deepcopy(arg2)
        self.arg3 = deepcopy(arg3)
        self.arg4 = deepcopy(arg4)
        self.arg5 = deepcopy(arg5)

    # attribute methods like getters and setters

Option 2: Let dataclasses do your work for you
As an alternative to hand-writing all of Attributes, I'd suggest making it a dataclass, which means you don't need to repeat the names, and allows you to define default_factorys for each field to generate default values on demand. That would allow you to write a simpler Attributes (with far less name repetition) matching what I wrote above like so:
from dataclasses import dataclass, field

@dataclass
class Attributes:
    arg1: list = field(default_factory=list)
    arg2: dict = field(default_factory=dict)
    arg3: set = field(default_factory=set)
    arg4: MyMutable = field(default_factory=MyMutable)
    arg5: OtherMutable = field(default_factory=OtherMutable)

    # attribute methods like getters and setters

and it will generate __init__ (as well as __repr__ and __eq__ for good measure; you can turn them off, or other features on, by parameterizing the @dataclass decorator) for you, including automatically calling your default_factory to initialize the field if and only if the caller did not provide the parameter.
Note: Unlike the other solution, this doesn't ensure caller-provided arguments are copied, so the risk of aliasing would remain. You could always define a __post_init__ to copy any fields you think this is likely to be a problem for.
Side-note: For my example, I just annotated the types as list, dict, and set; to annotate properly, you'd usually the typing classes (List, Dict, Set) and annotate with the types the containers are expected to hold, e.g. arg1: List[int] = field(default_factory=list) if arg1 is expected to be a list of ints.
Preferred solution (especially if the child must not have defaults, and the parent class should): dataclasses everywhere
In your example, Attributes() fails for lack of arguments, while Functionality() does not (substituting defaults),
dataclasses still cover that case adequately. You'd define both parent and child as dataclasses, but not provide defaults for the fields on the parent:
from dataclasses import dataclass, field

@dataclass
class Attributes:
    arg1: list
    arg2: dict
    arg3: set
    arg4: MyMutable
    arg5: OtherMutable

    # Optionally define __post_init__ to modify arguments, perform other work

    # attribute methods like getters and setters

@dataclass
class Functionality(Attributes):
    arg1: list = field(default_factory=list)
    arg2: dict = field(default_factory=dict)
    arg3: set = field(default_factory=set)
    arg4: MyMutable = field(default_factory=MyMutable)
    arg5: OtherMutable = field(default_factory=OtherMutable)

    # Optionally define __post_init__ to modify arguments, perform other work

    # Methods that give functionality to the class

Since no defaults are set on Attributes, direct use of Attributes will require you all arguments to be provided. But dataclasses are helpful again here; the fields of a child class of a dataclass are:

All of the fields of the parent(s)
Plus all of the fields of the child
When a given field is defined in both, the child's definition wins

So we can provide default_factorys solely on the child (and get defaulting behavior), but not on the parent (so it never defaults).


Populate dropdown B on selection in dropdown A On selection from drop-down A (HTML select) I need to populate another "sub"-dropdown with a different list of values. For instance, on selection of country in dropdown A, dropdown B should be filled with the list of cities of selected country.

This is my first attempt of writing such a thing in jQuery. Please point out areas for improvement, like more idiomatic or shorter way of achieving this result (without losing much readability if possible).

HTML:

<select id="Parentlist">
</select>

<select id="Sublist">
</select>

JavaScript + jQuery:

function populateParentlist(choice) {
       $('#choices').find('tr').each(function() {
           trowid = $(this).attr('id');
           $("#Parentlist").append('<option>' +trowid + '</option>');
       });
}

function populateSublist(choice) {
    $('#Sublist').find('option').remove();
    $('#choices').find('tr').each(function() {
        trowid = $(this).attr('id');
        $(this).find('td').each(function() {
           tdid = $(this).attr('id');
            if(trowid != choice)
                return;
           $('#Sublist').append('<option>' + tdid + '</option>');
        });
    });
}

populateParentlist('Poland')
populateSublist('Poland')

$(function() {
    $("#Parentlist").change(function() {
        var parsel = $(this).val();
        populateSublist(parsel);
    });
});

jsfiddle

Don't use a <table> element to "hide" extra data in your markup. It's a terribly complex way of doing things, and it's pretty hacky to have large, hidden elements like that (and not a good semantic use of a table either).
Be consistent with your naming; Sublist is capitalized, but choices is lowercase.
Alphabetize the city names :)

With regard the JS: It's ok given the context, but the use of the table is the real culprit. Still there's at least 1 thing you should change:

Don't call the populate* before the page has loaded (you wait before adding the change handler, because the #ParentList element might not have loaded yet; that same timing issue would cause the populateParentlist function to fail)

Now, ideally, the city list should be available as JSON or something (loaded from the server for instance), but if you're going to store it in the HTML, you might as well use a select and subdivide it using optgroup elements:

<select id="cities">
  <optgroup label="Poland">
    <option selected>Krakow</option>
    <option>Warszawa</option>
  </optgroup>
  ...

As a bonus, the list will work even without javascript. And you can set the initially-selected city in the HTML.

It also contains all the option elements you'll need for the city dropdown; we just need to convert the optgroups into options for a 2nd dropdown:

$(function () { // wait for page to load
    var cityDropdown = $("#cities"),
        countryDropdown = $('<select></select>'), // create a country dropdown
        countries = []; // ordered list of countries

    // parse the nested dropdown
    cityDropdown.children().each(function () {
        var group = $(this),
            countryName = group.attr('label'),
            option;

        // create an option for the country
        option = $('<option></option>').text(countryName);

        // store the associated city options
        option.data('cities', group.children());

        // check if the country should be selected
        if( group.find(':selected').length > 0 ) {
            option.prop('selected', true);
        }

        // add the country to the dropdown
        countryDropdown.append(option);
    });

    // add the country dropdown to the page
    cityDropdown.before(countryDropdown);

    // this function updates the city dropdown based on the selected country
    function updateCities() {
        var country = countryDropdown.find(':selected');
        cityDropdown.empty().append(country.data('cities'));
    }

    // call the function to set the initial cities
    updateCities();

    // and add the change handler
    countryDropdown.on('change', updateCities);
});

Here's a demo


16 bit checksum calculation I need to calculate checksum using 16-bit ones' complement addition:

while(byte>0)  //len = Total num of bytes
{
    Word = ((Buf[i]<<8) + Buf[i+1]) + Checksum; //get two bytes at a time and  add previous calculated checsum value

    Checksum = Word & 0x0FFFF; //Discard the carry if any

    Word = (Word>>16);     //Keep the carryout for value exceeding 16 Bit

    Checksum = Word + Checksum; //Add the carryout if any

    len -= 2; //decrease by 2 for 2 byte boundaries
    i += 2;
}

 Checksum = (unsigned int)~Checksum;

The above code works fine, and if I understood the concept well (to add 16 bits), add the carry if any and then take the compliment.

Are there any improvements or corrections?

If the packet size is less than 32k words, then you do not need to add the carry until the end:

while(byte>0)  //len = Total num of bytes
{
    Checksum = ((Buf[i]<<8) + Buf[i+1]) + Checksum; //get two bytes at a time and  add previous calculated checsum value

    len -= 2; //decrease by 2 for 2 byte boundaries
    i += 2;
}

 Checksum = (Checksum>>16) + Checksum; //Add the carryout

 Checksum = (unsigned int)~Checksum;


Closure implementation I have custom cell of tableview which is used as header as well as normal cell.

In custom cell I have this properties

public var btnReplyTappedClousre:((CommentCell) -> (Void))?
public var btnBulbTappedClousre:((CommentCell) -> (Void))?
public var btnOtherReplyTapped:((CommentCell) -> (Void))?

And  Call it with

@IBAction func btnReplyTapped(_ sender: Any) {
    self.btnReplyTappedClousre?(self)
}

//--------------------------------------------------------------------------------

@IBAction func btnBulbTapped(_ sender: Any) {
    self.btnBulbTappedClousre?(self)
}

//--------------------------------------------------------------------------------

@IBAction func btnViewPreviousReplyTapped(_ sender: Any) {
    self.btnOtherReplyTapped?(self)
}

In view Controller i have implemented that closure

func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
    let cell =  tableView.dequeueReusableCell(withIdentifier: StoryBoard.Cells.CommentCell, for: indexPath) as! CommentCell
    cell.isForHeaderCell = false
    cell.btnReplyTappedClousre = {[weak self] (cell) in
        self?.btnViewAllReplyTapped(cell)
    }
    cell.btnBulbTappedClousre = {[weak self] (cell) in
        self?.btnBulbTapped(cell)
    }
    return cell
}

func tableView(_ tableView: UITableView, viewForHeaderInSection section: Int) -> UIView? {
    let cell =  tableView.dequeueReusableCell(withIdentifier: StoryBoard.Cells.CommentCell) as! CommentCell
    cell.isForHeaderCell = true

    cell.btnReplyTappedClousre = {[weak self] (cell) in
        self?.btnViewAllReplyTapped(cell)
    }
    cell.btnBulbTappedClousre = {[weak self] (cell) in
        self?.btnBulbTapped(cell)
    }

    cell.btnReplyTappedClousre = {[weak self] (cell) in
        self?.btnReplyTapped(cell)
    }

    return cell

}

Is there any better way to do it ? Or can i improve it ?

Personally I never mutate the cell inside the cellForRowAt func but I send it a bunch of things that it needs to (re)build itself through a configure function. So:

cell.configure(with: isForHeaderCell, whenA: { /* someblock */ }, whenB: { /* another block */ }) /* etcetera */

You seem to be forwarding a lot of calls that are doing nothing except forwarding. So the only thing you really need to do is to forward the function you want to call in the end. What you need to understand is that functions can be parameters where blocks are asked and vice versa. A block is an anonymous function and if the in/out matches you can use it.

So technically you only need to pass the function that is going to do the required work all the way up and you're good. For example I have a class that fetches some data:

DataFetcher.fetch(for: id, whenDone: { result in /* etc */ })

This could be wrapped in a block:

{ (id: Int) in
    DataFetcher.fetch(for: id, whenDone: { result in /* etc */ })
}

And this block could be passed all the way up the chain, so you call down once.

func configure(with fetchStuff: (Int) -> ())

I also don't understand why you pass the Cell itself as a parameter. Perhaps because the way the @IBAction functions are structured by default but it's better to ignore it unless you have some really pressing reason to got back to exactly that cell.

Last but not least, providing defaults is not that bad when dealing with blocks that almost surely will be set during runtime:

public var btnReplyTappedClousre:((CommentCell) -> (Void))?

Could be:

private var replyAction: (Int) -> () = { _ in
    assertionFailure("This block should always be replaced by the configure function before the cell is shown")
}

Let me explain the changes in detail:

It can be private because it's set through your configure function and will only ever be called from within the cell. Better encapsulation.
It does not relate to the button but to what actually happens when you tap the button. So the @IBAction func replyButtonTapped() will call replyAction(chatId)
It only passes the required data (in this case the chat id). Nobody outside of CommentCell needs to know anything about CommentCell
The empty block containing only the assertionFailure will warn any boneheaded colleague that did not call configure during testing but will not crash when released (if it managed to escape during testing)
The block will always be set so you're not dealing with optionals anymore


Implement operation and simulation mode in an interface Context:

I have to create an interface between devices and a client.
My package is composed of:

Device.py
Error.py
DeviceA.py (for example)
DeviceB.py
...

Basically each DeviceA, DeviceB, ... inherit Device class. Device is composed of one FSM (state machine: module Fysom) and does several thing about communication, parsing, ... The purpose is to have one common FSM (same rules) for all devices but each device follow his own FSM. That's why there is one thread per device (Device inherit a Threading module).
A code simplified:

# One common state rule
TableFSM = {
'initial' : 'none',
'events': [
    {'name': 'do_start', 'src': ['OFF', 'none'], 'dst': 'LOADED'},
    {'name': 'do_stop', 'src': ['LOADED'], 'dst': 'OFF'},
    # same for resume, pause, ...
]

class Device(MyThread):
    def __init__(self):
        # FSM is instantiate here so ONE FSM per device with same rule
        self.fsm = Fysom(TableFSM)
        self.mode = "operation" #or simulation
    def start(self):
        self.fsm.do_start()
        # Real common Device parsing stuff
    def stop(self):
        self.fsm.do_stop()
        # Real common Device parsing stuff
    def pause(self):
        self.fsm.do_pause()
        # Real common Device parsing stuff
    def resume(self):
        self.fsm.do_resume()
        # Real common Device parsing stuff

class DeviceA(Device):
    def __init__(self):
        Device.__init__(self)
    def start(self):
        #Specific Real Device stuff
        super(DeviceA, self).start()
    def stop(self):
        #Specific Real Device stuff
        super(DeviceA, self).start()
    #def ...

class DeviceB(Device):
    def __init__(self):
        Device.__init__(self)
    def start(self):
        #Specific Real Device stuff
        super(DeviceB, self).start()
    def stop(self):
        #Specific Real Device stuff
        super(DeviceB, self).start()
    #def ...

Here methods called will be DeviceA().start() or DeviceB().stop() (bad represention) for example.

Issue:

I have to implement for each device an operation and simulation mode
For example  I could add in each method of each device class the following statement:

if self.mode == "operation":
    #some stuff
elif self.mode == "simulated":
    #almost nothing

I want to write it as generic as possible.
From a previous post I could derivate SimuDevice from Device with the same methods. Basically SimuDevice will only trigger the FSM :

class DeviceSimu(Device):
    def __init__(self):
        Device.__init__(self)
    def start(self):
        self.fsm.do_start()
    def stop(self):
        self.fsm.do_stop()
    def pause(self):
        self.fsm.do_pause()
    def resume(self):
        self.fsm.do_resume()

Question:

My issue is about instanciation and object. I want to be able to switch between operation and simulation mode without deleting and reinstantiate an object.

I can't understand how DeviceA could access to DeviceSimu method?
Is it a good way (conceptually) to do simulation and operation mode? Maybe I have to change the link between class.
Any other advice ?

You could simply have if-statements in each method of your class. This would probably result in (slightly) lesser lines of code than my suggestions. However, I think that solution has a lot of repeated code and is quite static. What if you wanted a device that simply simulates? Or one that simply operates?

My suggestion is to abstract the classes a bit more and have seperate classes for each mode. Then have a final DualModeDevice class that inherits from both of them:

class OperationDevice(Device):
    def __init__(self):
        super(OperationDevice, self).__init__()

    def operation_start(self):
        # Do operation specific code
        self.start()

    def operation_stop(self):
        # Do operation specific code
        self.stop()

    def operation_pause(self):
        # Do operation specific code
        self.pause()

    def operation_resume(self):
        # Do operation specific code
        self.resume()

class SimulationDevice(Device):
    def __init__(self):
        super(SimulationDevice, self).__init__()

    def simulation_start(self):
        # Do simulation specific code
        self.start()

    def simulation_stop(self):
        # Do simulation specific code
        self.stop()

    def simulation_pause(self):
        # Do simulation specific code
        self.pause()

    def simulation_resume(self):
        # Do simulation specific code
        self.resume()

class DualModeDevice(OperationDevice, SimulationDevice):
    def __init__(self, mode='simulation'):
        super(DualModeDevice, self).__init__()
        self._mode = mode

        # To be continued

Each function in SimulationDevice and OperationDevice needs to be named differently because of how Python interprets and resolves inheritence. If there is a identically named function from multiple inheritances, it will use the first available reference from the MRO (method resolution order).

Now with that being said, you could implement dual mode in two ways:

Function mapping

One way is to simply have a dict that holds a mapping from mode to function:

class DualModeDevice(OperationDevice, SimulationDevice):
    def __init__(self, mode='simulation'):
        super(DualModeDevice, self).__init__()
        self._mode = mode
        self._start_map = {
            'simulation': self.simulation_start,
            'operation': self.operation_start
        }
        # Repeat the above line for `stop`, `pause`, and `resume`.
        # Or you could combine them into a single `dict`.

    def stop(self):
        self._start_map[self._mode]()

    def stop(self):
        self._stop_map[self._mode]()

getattr

If you felt like being sneaky, you could use the getattr function (assuming consistent function names):

class DualModeDevice(OperationDevice, SimulationDevice):
    def __init__(self, mode='simulation'):
        self._mode = mode

    def run_command(self, command, mode=self.mode):
        getattr(self, '{}_{}'.format(mode, command))()

Since functions boil down to callable properties in classes, we can simply use getattr to get the correct function, then call it.


ASP.NET Core 3.1 AddHttpClient call that includes a call to the database So, I have an ASP.NET Core 3.1 web site that makes occasional calls to a vendor's RESTful API. I use the HttpClient class to perform these calls.

Currently, the web site is relatively low traffic (probably a couple of users per day). The current primary use case will result in 3 calls to the API for the complete workflow.

Even in previous versions of the framework, Microsoft recommended just creating a single instance of HttpClient and using it throughout the entire lifecycle of your application (to prevent port exhaustion). The new recommendation for ASP.NET Core is to register a service in your Startup class to act as a factory for your HttpClient. I initially used my own Singleton for but recently switched to follow Microsoft's recommendation.

My one concern is that I have to add my API Key as a header for each request, or it will be rejected.

I currently store the database key in SQL Server, using its always-encrypted feature to make sure that it stays private.

Here's the relevant logic in the ConfigureServices method in my Startup class:

services.AddDbContext<DatabaseContext>();

// Use TLS 1.3 for requests
// I'm a little uncertain if this is still the recommended way to configure this in ASP.NET Core
ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls13;

services.AddHttpClient("VendorAPICall", c =>
        {
            c.BaseAddress = new Uri("https://api.vendorname.com/");

            c.Timeout = new TimeSpan(4, 0, 0);

            c.DefaultRequestHeaders.Accept.Clear();

            c.DefaultRequestHeaders.Add("Accept", "application/json");

            // I store the API Key securely using SQL Server's Always Encrypted feature: https://docs.microsoft.com/en-us/sql/relational-databases/security/encryption/always-encrypted-database-engine?view=sql-server-ver15
            // I'm a little concerned about the potential performance impact of this, especially how
            // many extra database calls it could result in (particularly since I'm not doing
            // the async calls).
            using (var ctx = new DatabaseContext())
            {
                Security security = ctx.Security.First();

                // Header must include the API Token for all requests, or the request will be rejected
                c.DefaultRequestHeaders.Add("ApiToken", security.Apikey);
            }
        });

I have two points that I'm concerned about:

First, is the call to ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls13; still the recommended (or, at least, acceptable) way of doing this configuration in ASP.NET Core, or am I doing this wrong?

Secondly, I'm concerned about the database call and how many back-and-forths this could result in for the database. This seems a little unnecessary, since the API Key will only be renewed every 6 months or so, so it seems wasteful at a minimum and a potential scalability issue at worst.

I'm also a little annoyed that I'm not using the async version of the Entity Framework calls here. (I'm otherwise using the async version of all of the database calls wherever possible).

Am I worrying too much here, or is there actually a better way to do this? (I already established in a separate Q&A on Stack Overflow that I was, in fact, being paranoid about the possibility of keeping the API Key in memory, but that's probably a separate issue).

No need to manually create the DbContext. Since you have already registered it with the service collection, resolve it as needed.

AddHttpClient has an overload that provides access to the service provider.

services.AddHttpClient("VendorAPICall", (sp, c) => {
    c.BaseAddress = new Uri("https://api.vendorname.com/");
    c.Timeout = new TimeSpan(4, 0, 0);
    c.DefaultRequestHeaders.Accept.Clear();
    c.DefaultRequestHeaders.Add("Accept", "application/json");
    var ctx = sp.GetService<DatabaseContext>(); //<-- resolve context
    Security security = ctx.Security.First();
    // Header must include the API Token for all requests,
    // or the request will be rejected
    c.DefaultRequestHeaders.Add("ApiToken", security.Apikey);

});

This will allow the framework to manage its lifetime instead of constantly having to dispose of it yourself in a using block


Optimizing nested loop for calculating geodesic distance I segmented an image into N superpixels and constructed a graph based on that in which each superpixel is considered as a node. The information about neighbor superpixels is stored in glcms array. The weight between each pair of neighboring superpixels is stored in matrix W.

Finally, I want to compute geodesic distance between non-adjacent superpixels using graphshortestpath function. The following code does the mentioned process, however it is very time-consuming. Specifically, the last section of the code that computes geodesic distance takes more time than expected (more than 15 sec).

    Img=imread('input.jpg');
    [rows, columns, numberOfColorChannels] = size(Img);
    [L,N] = superpixels(Img,250);

    %Identifying neighborhood relationships
    glcms = graycomatrix(L,'NumLevels',N,'GrayLimits',[1,N],'Offset',  [0,1;1,0]); %Create gray-level co-occurrence matrix from image
    glcms = sum(glcms,3);    % add together the two matrices
    glcms = glcms + glcms.'; % add upper and lower triangles together, make it symmetric
    glcms(1:N+1:end) = 0;    % set the diagonal to zero, we don't want to see "1 is neighbor of 1"

    data = zeros(N,3);
    for labelVal = 1:N
    redIdx = idx{labelVal};
    greenIdx = idx{labelVal}+numRows*numCols;
    blueIdx = idx{labelVal}+2*numRows*numCols;
    data(labelVal,1) = mean(Img(redIdx));
    data(labelVal,2) = mean(Img(greenIdx));
    data(labelVal,3) = mean(Img(blueIdx));

    end

   Euc=zeros(N);
% Euclidean Distance
for i=1:N
    for j=1:N
        if glcms(i,j)~=0
            Euc(i,j)=sqrt(((data(i,1)-data(j,1))^2)+((data(i,2)-data(j,2))^2)+((data(i,3)-data(j,3))^2));
        end
    end
end

W=zeros(N);
W_num=zeros(N);

W_den=zeros(N);
OMG1=0.1;
for i=1:N
    for j=1:N
        if(Euc(i,j)~=0)
         W_num(i,j)=exp(-OMG1*(Euc(i,j)));
         W_den(i,i)=W_num(i,j)+W_den(i,i);
        end
    end
end

for i=1:N
    for j=1:N
         if(Euc(i,j)~=0)
             W(i,j)=(W_num(i,j))/(W_den(i,i));   % Connectivity Matrix W

         end
    end
end

s_star_temp=zeros(N);   %temporary variable for geodesic distance measurement
W_sparse=zeros(N);
W_sparse=sparse(W);
for i=1:N
    for j=1:N
        if W(i,j)==0 & i~=j;
            s_star_temp(i,j)=graphshortestpath(W_sparse,i,j); % Geodesic Distance
        end
    end
end

The question is how to optimize the code to be more efficient , i.e. less time-consuming.

I'll first go through the different section and outline possible improvements, then give some general comments at the end. I am not checking the algorithm for correctness, as I don't know it.

Img=imread('input.jpg');
[rows, columns, numberOfColorChannels] = size(Img);
[L,N] = superpixels(Img,250);

%Identifying neighborhood relationships
glcms = graycomatrix(L,'NumLevels',N,'GrayLimits',[1,N],'Offset',  [0,1;1,0]); %Create gray-level co-occurrence matrix from image
glcms = sum(glcms,3);    % add together the two matrices
glcms = glcms + glcms.'; % add upper and lower triangles together, make it symmetric
glcms(1:N+1:end) = 0;    % set the diagonal to zero, we don't want to see "1 is neighbor of 1"

You need to have defined idx by now, but you haven't, so the code doesn't run. Based on the superpixels documentation, you need to add idx = label2idx(L);

data = zeros(N,3);
for labelVal = 1:N
    redIdx = idx{labelVal};
    greenIdx = idx{labelVal}+numRows*numCols;
    blueIdx = idx{labelVal}+2*numRows*numCols;
    data(labelVal,1) = mean(Img(redIdx));
    data(labelVal,2) = mean(Img(greenIdx));
    data(labelVal,3) = mean(Img(blueIdx));
end

This is fine, it probably is fine to just write data(labelVal,1) = mean(Img(idx{1:N})), etc. but the performance should be similar.

Euc = zeros(N);
% Euclidean Distance
for i=1:N
    for j=1:N
        if glcms(i,j)~=0
            Euc(i,j)=sqrt(((data(i,1)-data(j,1))^2)+((data(i,2)-data(j,2))^2)+((data(i,3)-data(j,3))^2));
        end
    end
end

This can be replaced by Euc = pdist2(data,data).*(glcms~=0);

W=zeros(N);
W_num=zeros(N);

W_den=zeros(N);
OMG1=0.1;
for i=1:N
    for j=1:N
        if(Euc(i,j)~=0)
         W_num(i,j)=exp(-OMG1*(Euc(i,j)));
         W_den(i,i)=W_num(i,j)+W_den(i,i);
        end
    end
end

This can be replaced by W_num = exp(-OMG1*Euc).*(Euc~=0); and W_den = sum(W_num); (here W_den is a vector, not a matrix, but that's what you want because you were only using diagonal elements of your W_den matrix).

for i=1:N
    for j=1:N
         if(Euc(i,j)~=0)
             W(i,j)=(W_num(i,j))/(W_den(i,i));   % Connectivity Matrix W
         end
    end
end

This can be replace by W = W_num./W_den.'; and W(isnan(W)) = 0;.

s_star_temp=zeros(N);   %temporary variable for geodesic distance measurement
W_sparse=zeros(N);
W_sparse=sparse(W);
for i=1:N
    for j=1:N
        if W(i,j)==0 & i~=j;
            s_star_temp(i,j)=graphshortestpath(W_sparse,i,j); % Geodesic Distance
        end
    end
end

Here you define W_sparse twice, you don't need to initialise it with zeros(N). This part is finding all shortest paths, and there is a builtin for that (which is probably a lot more efficient that generating the paths independently). Replace this whole section with s_star_temp = graphallshortestpaths(W).*(W==0); This seems to speed it up from ~15 seconds to 0.005 seconds.

So this is how I would write your algorithm

Img = imread('input.jpg');
[rows, columns, numberOfColorChannels] = size(Img);
[L,N] = superpixels(Img,250);

idx = label2idx(L);

%Identifying neighborhood relationships
glcms = graycomatrix(L,'NumLevels',N,'GrayLimits',[1,N],'Offset',  [0,1;1,0]); %Create gray-level co-occurrence matrix from image
glcms = sum(glcms,3);    % add together the two matrices
glcms = glcms + glcms.'; % add upper and lower triangles together, make it symmetric
glcms(1:N+1:end) = 0;    % set the diagonal to zero, we don't want to see "1 is neighbor of 1"

data = zeros(N,3);
for labelVal = 1:N
    data(labelVal,1) = mean(Img(idx{labelVal}));
    data(labelVal,2) = mean(Img(idx{labelVal}+rows*columns));
    data(labelVal,3) = mean(Img(idx{labelVal}+2*rows*columns));
end

Euc = sparse(pdist2(data,data).*(glcms~=0));

OMG1 = 0.1;

W_num = exp(-OMG1*Euc).*(Euc~=0);
W_den = sum(W_num);

W = W_num./W_den.';
W(isnan(W)) = 0;

s_star_temp = graphallshortestpaths(W).*(W==0);

Notice that I made Euc a sparse matrix. This makes the resulting matrices (W_num, W_den, W) also sparse. Your original code left them as full matrices, which was unnecessary. This code runs in about 1.3 seconds on my machine, compared to about 16 for your original code (with the picture I used).

Minor general comments on your code:

Use spaces around your = signs, it makes things easier to read
Try to avoid very long lines, for example the graycomatrix line with its comment will go over the end of the window and be lost
Write more comments
Matlab best practice is to avoid using i and j for loop variables, prefer ii and jj. It's not very important, but i and j can also mean the imaginary unit.


Deleting duplicate elements from a single-linked list in C++11 The code below is a simple coding exercise I got from a book that asks to remove the duplicates from a single-linked list (using a simple struct; this is part 2, see part 1 here).

Question 1: Is there a better way to delete a Node than by creating a temporary variable temp and deleting that. I actually tried deleting p directly and it worked. I find this surprising because after I delete p I would expect p->next to be a segmentation fault. Is this a compiler-specific trickery or is it a safe practice?

Question 2: I'm a little confused about passing pointers by reference. If I don't pass head by reference in insert, it doesn't work; but passing them by copy to remove_duplicates works (that is, the second call to print_node doesn't print the original list). Why?

#include <iostream>
#include <unordered_map>

struct Node {
  int data;
  Node* next;
};

void insert(Node*& head, const int data) {
  head = new Node {data, head};
}

void print_node(Node* head) {
  if (!head) return;

  std::cout << head->data;
  for(auto p = head->next; p; p = p->next) {
    std::cout << "->" << p->data;
  }
  std::cout << std::endl;
}

void remove_duplicates(Node * head) {
  if (!head || !head->next) return;

  std::unordered_map<int, bool> map {{head->data, true}};
  for (auto p = head->next, prev = head; p; p = p->next) {
    if (map[p->data]) {
      prev->next = p->next;
      Node* temp {p};
      delete temp;
    } else {
      map[p->data] = true;
      prev = p;
    }
  }
}

int main(int argc, char* argv[]) {
  Node* my_list {};
  insert(my_list, 2);
  insert(my_list, 1);
  insert(my_list, 3);
  insert(my_list, 2);
  insert(my_list, 2);
  insert(my_list, 1);
  insert(my_list, 4);
  print_node(my_list);
  // 4->1->2->2->3->1->2
  remove_duplicates(my_list);
  print_node(my_list);
  // 4->1->2->3
}

Currently your code is broken (it just happens to work accidentally).
  for (auto p = head->next, prev = head; p; p = p->next) {
    if (<Test>) {
      <Stuff>
      Node* temp {p};    // So p and temp both point at the same block of memory
      delete temp;       // You delete the memory pointed at by temp.
                         // This is the same effect as deleting p.
                         // This memory now belongs to the system and not
                         // your code. Access anything within this memory
                         // block is "undefined behavior".

                         // When you loop back to the `for` and
                         // execute `p = p->next` you are now
                         // invoking undefined behavior.

The term "undefined behavior" basically means the standard does not define what can happen and any hardware/compiler can do anything it likes as it result it can blow up or could look like it is working perfectly.
The reason the standard does not specify is to allow each implementation to pick the most efficient thing possible for them (you can specify the most efficient thing).
The block you have given back may be on a page that has been returned to the OS. The memory management could re-use parts of that block for its own internal buffers (to make allocation/deallocation efficient). The content of this block could have any value in it or it could be completely removed from the memory available to your processes. The least worst that could happen is probably a segfault and your application crashes. The real worst is that your application gets a random pointer and seems to work normally (but in reality it is trashes memory all through your application making it really hard to debug). Even much worse is that it happens to actually work in your tests then breaks when you push to production.
Fixed code
  std::unordered_map<int, bool> map {{head->data, true}};

  for (auto p = head->next, prev = head; p;) {
      if (map[p->data]) {
          prev->next = p->next;
          Node* temp {p};
          p = p->next;         // Move p before you delete its memory
          delete temp;         // Now you delete temp (which points at the old value)
      }
      else
      {
          map[p->data] = true;
          prev = p;
          p = p->next          // Don't forget you need to move p in both branches.
      }
  }

Bracing style.
This is horrible (IMO)
} else {

It is also not a common bracing style used in C++ os better avoided. The two main common styles are
if (<test>) {
}
else {
}

// or

if (<test>)
{
}
else
{
}

Questions

Is there a better way to delete a Node than by creating a temporary variable temp and deleting that.

No. Because you need to use the next member before deleting. So you have moved your pointer. So you need the temp value to record where you where so that you can delete it.

I actually tried deleting p directly and it worked.  I find this surprising because after I delete p I would expect p->next to be a segmentation fault.

It could be a segfault or it could work as expected. The memory does not belong to you so you can't know what is there anymore. But the standard does not require the code to do anything special with the memory so it could just contain the values it used to have before you called delete.

Is this a compiler-specific trickery or is it a safe practice?

Its definitely not safe to even read this memory. It is just happens to work. But if you change anything it may stop working. Compiler flags/ Compiler/ OS Version/ OS/ Hardware. Any or none of these things could trigger a change in behavior.

I'm a little confused about passing pointers by reference. If I don't pass head by reference in insert, it doesn't work; but passing them by copy to remove_duplicates works (that is, the second call to print_node doesn't print the original list).

The remove_duplicates() only works as long as you don't try and remove the value of the head node. But because the head is never a duplicate it will work.
A reference is a variable alias. ie. It is another name for a variable that already exists. So by changing the value of the reference you actually change the originals value.
int  head  = 6;
int& otherHead = head;   // otherHead is an alias for head.
                         // If I change the value of otherHead then
                         // I change the value of head.

otherHead = 12;          // head is now 12.

So in the insert() function you are passing head by reference (it becomes another name for the variable you passed as the parameter).
void insert(int& otherValue)
{
    otherValue = 15;  // otherValue is an alternative name for the parameter
                      // that was passed to this function.
}
int main()
{
    int value = 5;
    insert(value);
    std::cout << value;  // prints 15
}

So in your function:
void insert(Node*& head, const int data) {
  head = new Node {data, head};
  ^^^^  // This is an alias for another variable.
        // So we are changing the value of that other variable.
}

If we don't use a reference:
void broken_insert(Node* head, const int data) {
  head = new Node {data, head};
  ^^^^  // This is a local variable (a copy of the original)
        // We change head here. But it does not change the original variable.
}
int main()
{
    Node* data = nullptr;
    broken_insert(data, 12);
    std::cout << data << "\n"; // Still prints nullptr
}                              // as broken_insert only changed the local
                               // variable head it has nothing to do with
                               // the variable data.


Awkward Generic Repository Call I am implementing a generic repository pattern. I have done this quite a few times now but every time I do it there is something bugging me.

If I have a database design like below. (All the tables relate to one another.)
And I make a call like the following:

public IEnumerable<Domain> GetRealEstate()
{
     return _repository.GetAll();
}

I can get all the models from just that one call (The wonders of EF). The thing that bugs me is the fact that I have to say Domain in the method call, from the domain entity I will get all the relevant entity (Lazy loading) Companies etc. etc. It just feels wrong to use domain entity to get all the companies etc. etc. The repo pattern that I am using is a straight forward one.

Is there a better way of writing the methods so that it does not look so weird?

Controller
RealEstateController

[RoutePrefix("api/realestate")]
    public class RealEstateController : ApiController
    {
        private readonly IRealEstateService _service;

        public RealEstateController(IRealEstateService service)
        {
            _service = service;
        }

        [Route("")]
        public Task<Domain> GetRealEstates()
        {
            var collection =  _service.GetRealEstate();
            return null;
        }

        [Route("{domainName}")]
        public Task<Domain> GetRealEstate(string domainName)
        {

        }
    }

Service

public class RealEstateService : IRealEstateService
    {
        private readonly IRealEstateRepository _repository;
        public RealEstateService(IRealEstateRepository repository)
        {
            _repository = repository;
        }
        public IEnumerable<Domain> GetRealEstate()
        {
            return _repository.GetAll();
        }
    }

I know this is not 100% spot on, but you should be able to take something from it and apply it in your own repository.

Here's a piece of code I use for one of my EF repositories, which I find very useful. I use a lot of Razor templating which won't lazy-load entities, so whenever I need to eagerly-load, I just include whatever relational entities I need before passing it to razor.

    public IQueryable<T> FindByInclude<T>(Expression<Func<T, bool>> predicate, params Expression<Func<T, object>>[] include) where T : class
    {
        IQueryable<T> query = _context.Set<T>().Where(predicate);

        foreach (var item in include)
        {
            query = query.Include(item);
        }

        return query;
    }

Example:

var user = Repo.FindByInclude<User>(z=>z.Name = "Joachim Nordvik", "Address","Properties.Extended");

Now both the Address relation and Properties.Extended relations are eagerly loaded.


Maths quiz that read/writes CSV files Could this code possibly be shortened, or even better be rewritten in the fewest lines of code possible?

import csv

import random
import math
import operator as op

name=input("What is your name: ")
print("Hello there",name,"Welcome to the math quiz, you will be asked 10 random questions on Addition, Multiplication and Subtraction")

choice = random.choice(['x','-','+'])
finish = False
questionnumber = 0
correctquestions = 0
Class = ""

while finish == False:
       choice = random.choice(['x','-','+'])
       if questionnumber < 10 | questionnumber >= 0:
           number1 = random.randrange(1,10)
           number2 = random.randrange(1,10)
           print((number1),(choice),(number2))
           answer=int(input("What is the answer?"))
           questionnumber = questionnumber + 1

        if choice==("+"):
            realanswer = number1+number2
            if answer==realanswer:
                print("Well done that's the correct answer")
                correctquestions = correctquestions + 1
            else:
                print("Unlucky wrong answer")

        if choice==("x"):
            realanswer = number1*number2
            if answer==realanswer:
                print("That's the correct answer")
                correctquestions = correctquestions + 1
            else:
                print("That was not correct")

        elif choice==("-"):
            realanswer = number1-number2
            if answer==realanswer:
                print("That's the correct answer")
                correctquestions = correctquestions + 1
            else:
                print("Unlucky not the corret")

    else:
        finish = True
        print("Well done You have completed the math quiz, Your score was " + str(correctquestions) + "/10 questions.")

while Class not in ("1" , "2" , "3"):
    Class = input("whish class are you in?")

ScoresList = []

if Class == "1":
    with open("class1.csv","r") as class1:
        reader = csv.reader(class1)
        for row in reader:
            ScoresList.append(row)
elif Class == "2":
    with open("class2.csv" , "r") as class2:
        reader = csv.reader(class2)
        for row in reader:
            ScoresList.append(row)

elif Class == "3":
    with open("class3.csv","r") as class3:
        reader = csv.reader(class3)
        for row in reader:
            ScoresList.append(row)
else:
    print("Please enter a valid class")

position = 0
match = 0

for row in ScoresList:

    if len(row) > 0 and row[0] == name:
        match = 1
        break
    else:
        position += 1

print(position , match)
if match == 0:
    ScoresList.append([name,str(correctquestions)])
else:

    row = ScoresList[position]
    if len(row) == 4:
        del row[1]
    row.append(str(correctquestions))

for row in ScoresList:
    print(row)

if Class == "1":
    with open ("class1.csv" , "w", newline='') as file:
        writer = csv.writer(file)
        for row in ScoresList:
            writer.writerow(row)

if Class == "2":
    with open ("class2.csv" , "w", newline='') as file:
        writer = csv.writer(file)
        for row in ScoresList:
            writer.writerow(row)

if Class == "3":
    with open ("class3.csv" , "w", newline='') as file:
        writer = csv.writer(file)
        for row in ScoresList:
            writer.writerow(row) `

You surely can shorten it by also improving readability

For example:

if Class == "1":
    with open ("class1.csv" , "w", newline='') as file:
        writer = csv.writer(file)
        for row in ScoresList:
            writer.writerow(row)

if Class == "2":
    with open ("class2.csv" , "w", newline='') as file:
        writer = csv.writer(file)
        for row in ScoresList:
            writer.writerow(row)

if Class == "3":
    with open ("class3.csv" , "w", newline='') as file:
        writer = csv.writer(file)
        for row in ScoresList:
            writer.writerow(row) `

Becomes:

if Class in ("1", "2", "3"):
    with open ("class{}.csv".format(Class) , "w", newline='') as file:
        writer = csv.writer(file)
        for row in ScoresList:
            writer.writerow(row) `

And, as you do that twice, you can extract it into a function to further avoid duplication.

You can also extract common code out of conditional branches

        if answer==realanswer:
            print("That's the correct answer")
            correctquestions = correctquestions + 1
        else:
            print("Unlucky not the corret")

is repeated in all of the ifs so you can write it just once at the bottom.


Mean of many subsets of a dataframe I have large dataframe containing many replicates.
The replicates are in groups of 3. So the first 3 replicates are in column 1, 2 and 3. The second set 4, 5 and 6... and so on.

Now I create a new dataframe containing for each set of replicates the mean.

The code below works, but it is really clumpy and especially the cbind and the collumname setting is really ugly.

# first i create the new dataframe
data.mean<- data.frame(matrix(nrows=30))

# iterate over every third collumn
for(col in seq(1,length(colnames(data)), by=3)){

    # create a subset from the dataframe and compute the mean of the rows and finally cbind it to the result dataframe
    data.mean <-cbind(data.mean,apply(subset(data, select=seq(col,length.out =   3)),1,mean, na.rm = TRUE))

    # setting the new collumn name to the colname from the old dataset (name of the first replicate)
    colnames(data.mean)[ncol(data.mean)] <- colnames(data)[col]
}

I really want to improve my R coding style so i am happy about every tip!

Here is a proposal for a different approach that doesn't use a for loop and has some simplifications.

First, an example data frame:

dat <- data.frame(a1 = 9:11, a2 = 2:4, a3 = 3:5,
                  b1 = 4:6, b2 = 5:7, b3 = 1:3)
#   a1 a2 a3 b1 b2 b3
# 1  1  2  3  4  5  6
# 2  2  3  4  5  6  7
# 3  3  4  5  6  7  8

Now, we set the number of columns per group:

# number of columns per group (1-3, 4-6)
n <- 3

Based on this information, some necessary information can be calculated:

# number of groups
n_grp <- ncol(dat) / n
# 2

# column indices (one vector per group)
idx_grp <- split(seq(dat), rep(seq(n_grp), each = n))
# $`1`
# [1] 2 3 4
#
# $`2`
# [1] 5 6 7

In the next step, lapply is used to calculate the row means of each group. This is much more convenient with the rowMeans function.

# calculate the row means for all groups
res <- lapply(idx_grp, function(i) {
    # subset of the data frame
    tmp <- dat[i]
    # calculate row means
    rowMeans(tmp, na.rm = TRUE)
})
# $`1`
# [1] 4.666667 5.666667 6.666667
#
# $`2`
# [1] 3.333333 4.333333 5.333333

The command above returns a list. It can be transformed into a data frame:

# transform list into a data frame
dat2 <- as.data.frame(res)
#         X1       X2
# 1 4.666667 3.333333
# 2 5.666667 4.333333
# 3 6.666667 5.333333

In order to set the column names of the new data frame, we first have to extract the column names of the groups' first columns.

# extract names of first column of each group
names_frst <- names(dat)[sapply(idx_grp, "[", 1)]
# [1] "a1" "b1"

Now, these names are used for the new data frame:

# modify column names of new data frame
names(dat2) <- names_frst
#         a1       b1
# 1 4.666667 3.333333
# 2 5.666667 4.333333
# 3 6.666667 5.333333

Done.


Optimizing a recursion code over lists I'm writing three functions which iterate over lists, I want do this recursively, but I think this code is not as efficient as it could be.

def flatten(lst):

    if type(lst)==int:
        return [lst]
    elif lst==[]:
        return lst
    else:
        return sum([flatten(sub) for sub in lst], [])

def size(lst): #counts how many numbers and pairs of parentheses in the list
    global cnt
    cnt=0
    if type(lst)==int:
        cnt+=1
        return 1

    if lst==[]:
        cnt+=1
        return 1
    else:
        return (sum([size(sub) for sub in lst])+cnt)

def transform(f, lst):
    if lst==[]:
        return []
    if isinstance(lst,(int,float)):
        return f(lst)
    else:
        return [transform(f,sub) for sub in lst]

The flatten function will not handle any element other than integer or list. A better and simpler way of handling it is using a predefined function flatten.

import compiler.ast as cmp
flatten = cmp.flatten

>>> flatten([1,2,[3,[4]]])
>>> [1,2,3,4]

Now the size function can be rewritten as :

def size(lst):
    return len(lst) + sum(
        nested_count(l) for l in lst if isinstance(l,list))

I also noticed that you take care of float in transform function and not in (your) flatten function. Please ensure all cases or add catch Exceptions where you expect it.


Finding an element in an array that occurs a given number of times I am trying to solve the below problem from an online coding site:

  Fredo is pretty good at dealing large numbers. So, once his friend
  Zeus gave him an array of N numbers, followed by Q queries which he
  has to answer. In each query, he defines the type of the query and the
  number f for which Fredo has to answer. Each query is of the following
  two types:

  Type 0: For this query, Fredo has to answer the first number in the array (starting from index 0) such that its frequency is at least
  equal to f.
  Type 1: For this query, Fredo has to answer the first number in the array such that frequency is exactly equal to f.

  Now, Fredo answers all his queries but now Zeus imagines how he should
  verify them. So, he asks you to write a code for the same.

  Note: If there is no number which is the answer to the query, output
  0. Use fast I/O.

  Input:

  The first line of the input contains N, the size of the array
  The next line contains N space separated integers
  The next line contains Q, denoting the number of queries

  Then follow Q lines, each line having two integers type and f,
  denoting the type of query and the frequency for which you have to
  answer the query.

  Output:

  You have to print the answer for each query in a separate line.

  Input constraints:

  \$1 \le N \le 10^6\$
  \$1 \le A[i] \le 10^{18}\$
  \$1 \le Q \le 10^6\$
  \$0 \le \text{type} \le 1\$
  \$1 \le f \le 10^{18}\$

  Sample input:

6
1 2 2 1 2 3
5
0 1
0 2
1 2
1 3
0 3

  Sample output:

1
1
1
2
2

Here is the solution that I have tried:

from collections import Counter
import sys
tokenizedInput = sys.stdin.read().split()
t = int(tokenizedInput[0])
a = []
for i in range(t):
    s = int(tokenizedInput[i+1])
    a.append(s)
collection = Counter(a)
key = collection.keys()
value = collection.values()
q = int(tokenizedInput[i+2])
k = i+2
for j in range(q):
    query = int(tokenizedInput[k+2*j+1])
    f = int(tokenizedInput[k+2*j+2])
    temp1 = list(key)
    temp2 = list(value)
    for w in range(t):
        try:
            index = temp1.index(a[w])
        except:
            continue
        if query == 0:
            if temp2[index] >= f:
                print a[w]
                break
            else:
                del temp1[index]
                del temp2[index]
        else:
            if temp2[index] == f:
                print a[w]
                break
            else:
                del temp1[index]
                del temp2[index]
        if len(temp1) == 0:
            print 0
            break
        if w == t-1:
            print 0

This code runs properly and gives the correct output for smaller test cases but crosses the time limit on larger test cases. Can someone please suggest what improvements can be made to this code to improve the speed.

Style

Codding

I suggest you to make use of white space so your code is more readable; as it stand, it seems like a bunch of thoughts thrown away without even trying to organize them. Read PEP 8 too, which will make you code look a bit more like Python to people that are used to read some Python code.

It is also good practice to organize your code into functions, so you can extract out logical steps, name them and, as such, make it clearer to everyone what it is you are trying to accomplish. Read about PEP 257 to make it even more clear.

At the very least, if you can't find out good functions, you may want to put all your top-level code into a main function and have, at the bottom of your script:

if __name__ == '__main__':
    main()

You can read a bit more about this idiom in this SO post.

Reading input

In challenges like that, where the input is highly regularly formatted, I have a tendency to favor raw_input rather than manipulating stdin directly.

It lets you more easily parse each line independently; for instance:

def parse_data():
    raw_input()  # Skip the first line containing number of elements
    items = raw_input()
    return items.split()

def run_queries(data):
    count = int(raw_input())
    for _ in range(count):
        type_, element = raw_input().split()
        result = compute_query(int(type_), element, data)
        print(result)

def compute_query(type_, item, data):
    # TODO

if __name__ == '__main__':
    data = parse_data()
    run_queries(data)

You will note that I don't try to convert the elements to integers as there is no added value doing so. Equality of two strings is as valid than equality of two integers to know that two elements are the same.

Algorithm

You know about Counter which is a good way to count elements from an iterable. You can use that at your advantage to reverse the keys and values so you know, for each "number of repetitions", which items corresponds.

from collections import Counter

def count_elements(array):
    counts = Counter(array)

    indexor = {}
    for element, count in counts.iteritems():
        indexor.setdefault(count, []).append(element)

    return indexor

There are several improvements to this snippet. First of, indexor will only let you answer queries of type 1 where you need the exact amount of elements. You can easily create a second dictionary to answer type 0 queries where you decrease count and append to each entry in the range \$[0, count]\$. The second improvement is to use a defaultdict(list) from collections to avoir the call to setdefault.

But the main thing is: this snippet can't answer the question as Counter is a dictionnary and thus doesn't take the order of insertions into account. Luckily, collections also provides OrderedDict which do. So your best friend for this challenge will most likely be:

from collections import Counter, OrderedDict

class OrderedCounter(Counter, OrderedDict):
    pass

Replace the use of Counter by OrderedCounter and voilà, you're done. Each query can be simplified to getting the right dictionnary based on the query type and then getting the first element of the list accessed by the element count.

Putting all that together

from collections import Counter, OrderedDict, defaultdict

class OrderedCounter(Counter, OrderedDict):
    pass

def count_elements(array):
    counts = OrderedCounter(array)

    strict_indexor = defaultdict(list)
    loose_indexor = defaultdict(list)

    for element, count in counts.iteritems():
        strict_indexor[count].append(element)
        for c in range(count, 0, -1):
            loose_indexor[c].append(element)

    return loose_indexor, strict_indexor

def parse_data():
    raw_input()  # Skip the first line containing number of elements
    items = raw_input()
    return items.split()

def run_queries(items_counts):
    queries_count = int(raw_input())
    for _ in range(queries_count):
        type_, number_of_elements = raw_input().split()
        result = compute_query(int(type_), number_of_elements, items_counts)
        print(result)

def compute_query(type_, count, items_counts):
    indexor = items_counts[type_]
    return indexor[count][0]  # Assume there is no query having no answer

if __name__ == '__main__':
    items = parse_data()
    counts_by_query_type = count_elements(items)
    run_queries(counts_by_query_type)


Practicing recursion with getElementsByClassName I'm trying to get a good grasp on recursion by implementing document.getElementsByClassName. I've made something that works but I'd like to know if there is some blatant refactoring that should be done or perhaps some not-so-edge cases that might break it.

var getElementsByClassName = function(className){
  var result = [];
  function diveIn(motherNode, result, className){
    var maNodes = motherNode.childNodes
    for (var i = 0; i < maNodes.length; i++){
        var classes = maNodes[i].classList || [];
        classes.forEach(function(classy){
            if (classy === className){
                result.push(maNodes[i]);
            }
        })
        if (maNodes[i].childNodes[0]){
            diveIn(maNodes[i], result, className);
        }
    }
  }
  diveIn(document, result, className);
  return result;
};

Apart from what cssGEEK pointed out, your code cannot work at all, since maNodes[i].classList doesn't return an Array but a DOMTokenList, which has no forEach() method.

So I'm surprised you said it works: it can happen only when no class is defined for an element (then your || [] makes classes to be an Array), but for each element having class(es) it fires an error "classes.forEach is not a function".

Here is a slighthly different code to achieve what you planned using the same way:

var getElementsByClassName = function(className){
  var result = [];
  function diveIn(motherNode, result, className){
    var maNodes = motherNode.childNodes
    for (var i = 0; i < maNodes.length; i++){
        var classes = maNodes[i].classList;
        if (classes && classes.contains(className)) {
            result.push(maNodes[i]);
        }
        if (maNodes[i].childNodes[0]){
            diveIn(maNodes[i], result, className);
        }
    }
  }
  diveIn(document, result, className);
  return result;
};

You can look at it working here.

But again don't forget that depending of how heavy is the HTML document it may encounter a max call stack size condition.


Get nth prime number using sieve of Eratosthenes I am tasked with being able to find the \$n\$th prime number. I've tried to implement something like the sieve of Eratosthenes in increments of 200. The code works and returns the \$n\$th prime number. However, when asking for the 1000th prime number I already notice a significant lag on my box.

This code needs to be able to quickly return the \$n\$th prime where \$n\$ is a massive number. For the challenge I only need to get up to 200,000. However, being able to finalize this for efficient use for up to a million would be pretty awesome, I think.

This is what I have working so far:

def nthPrime(ind): #gets nth prime number. IE: 5th prime == 11. works based off very in-efficient version of Sieve of Eratosthenes. but in increments of 200
    p={}
    T = 2
    incST = 2
    incEND = incST + 200
    lenfinal = 1

    while lenfinal <= ind:
        for i in range(incST,incEND):
            p[i] = True
        T=2
        while T <= math.sqrt(incEND):
            l = 0
            while l <= incEND:
                if T**2 + (T*l) >= incST:
                    p[T**2 + (T*l)] = False
                l+=1
                if T**2+(T*l) > incEND:
                   break

            for k,v in sorted(p.items()):
                if v and k > T:
                    T = int(k)
                    break

        incST = incEND + 1
        incEND = incST + 200
        lenfinal = sum(1 for k,v in p.items() if v)

    final = [k for k,v in sorted(p.items()) if v]
    return final[ind-1]

First a small stylistic review, followed by an alternative algorithm.

You calculate math.sqrt(incEND) many times, just calculate it once outside of the inner while loop. Similarly, you calculate T**2 + (T*l) two to three times per loop. Do it once and save it to a variable.

Python has an official style-guide, PEP8, which recommends using a space in a comma-separated argument list, so use range(incST, incEND) and for k, v in ...

It also recommends using lower_case_with_underscores for variables and function names and using a space before and after the = in variable assignments.

You did not include the import math, which you should do for completeness.

It is probably more efficient to take a prime number generator and count up to the nth prime than keeping a list of all prime numbers in memory.

For this I would use something like this, which I got from a python cookbook site. Alternative algorithms can be found, for example in this Stack Overflow post. It is basically also a Sieve of Eratosthenes, but one where the yielding and the marking-off are interleaved.

def primes():
    '''Yields the sequence of prime numbers via the Sieve of Eratosthenes.'''
    D = {}
    yield 2
    # start counting at 3 and increment by 2
    for q in itertools.count(3, 2):
        p = D.pop(q, None)
        if p is None:
            # q not a key in D, so q is prime, therefore, yield it
            yield q
            # mark q squared as not-prime (with q as first-found prime factor)
            D[q*q] = q
        else:
            # let x <- smallest (N*p)+q which wasn't yet known to be composite
            # we just learned x is composite, with p first-found prime factor,
            # since p is the first-found prime factor of q -- find and mark it
            x = p + q
            while x in D or x % 2 == 0:
                x += p
            D[x] = p

And then finding the nth prime becomes trivial:

def nth_prime(n):
    if n < 1:
        raise ValueError("n must be >= 1 for nth_prime")
    for i, p in enumerate(primes(), 1):
        if i == n:
            return p

This takes about 1 second to find the 200,000th prime (2750159) on my machine.


Unordered list element via templating I'm creating an unordered list element in Backbone via Underscore templating:

Hello World

I'm wondering two things:

Is setting style still considered bad practice when creating elements through templating?
Should I be setting the style using jQuery after I generate the HTML from the template? This would be done inside of Backbone's render instead of inside of the template.

Something like:

render: function () {

    this.$el.html(this.template(this.model.toJSON()));
    this.$el('#FrontRack').css({ backgroundImage: ... }); // Example

    return this;
}

Interesting questions.

Is setting 'style' still considered bad practice when creating elements through templating?

Only if you cannot reasonably use css classes (which is your case)
Should I be setting the style using jQuery after I generate the HTML from the template?

Since you know the height and size prior to generating, you should make this part of the template

Also, this :

Hello World

Only accesses data under data.get('racks').at(0), so that is what you should provide to the template ( as say, data ), then your template becomes

Hello World


Converting long list of if/else In order to convert a code such as:

Company company = new Company();
if(currentToken.equals("employee_list")) {
   company.setEmployees((ArrayList<Employee> parseEmployees(reader));
}
else if(currentToken.equals("contractor_list")) {
   company.setContractors((ArrayList<Contractor> parseContractors(reader));
}
else if(currentToken.equals("legal_name")) {
   company.setLegalName((String)reader.nextString());
}
else if(currentToken.equals("profit")) {
   company.setLegalName(Double.parseDouble((String)reader.nextString()));
} ...etc

I.e long list of if/else reading from a stream and populating the object I did the following:

public interface CustomSetter<T> {
  void setToObject(T o, Reader r);
}

HashMap<String, CustomSetter> setters = new HashMap<>();
setters.put("employee_list", new CustomSetter<Company>() {
     public void setObject(Company company, Reader reader) {
        company.setEmployees((ArrayList<Employee> parseEmployees(reader));
     }
 });

setters.put("contractor_list", new CustomSetter<Company>() {
     public void setObject(Company company, Reader reader) {
        company.setContractors((ArrayList<Contractor> parseContractors(reader));
     }
 });

etc.
Effectively doing:

setters.get(currentToken).setObject(company, reader);

So now I have all these initialized in my hashmap.
What do you think of this approach? Can it be improved?

I would assume that Company has e setter method signature as setEmployees(List employees) or setEmployees(Arraylist employees) or some ? wildcard setter, since it accepts as parameter List<Employee> and List<Contractor>.

I would also assume that Employee and Contractor extend one another or the same super class.

In that case, I don't see the benefit of creating such a custom generic setter. Instead I would use the same setter, and mitigate the type identification in the parsing method: company.setEmployees(parseFromReader(currentToken, reader))
and have the List<Employee> parseFromReader(currentToken, reader) worry about the specific instance type, since I assume also most of the information to be parsed would be the same (if the classes extend the same super class).


Check if a range in Excel contains DataValidation One of the answers to Determine if cell contains data validation
 got me thinking about an edge case I needed to solve. If cell happens to be a multi-cell range what's the best way to answer? The answer below returns False if the range contains both a cell with and without validation.

Function HasValidation(cell As Range) As Boolean
    Dim t: t = Null

    On Error Resume Next
    t = cell.Validation.Type
    On Error GoTo 0

    HasValidation = Not IsNull(t)
End Function

The answer I arrived at allows for non-contiguous ranges. I chose Variant as the return type in the case that there's a mix of validated and non-validated cells. Since it's neither True nor False a Null seemed the best fit.

Public Function HasValidation(ByVal cell As Range) As Variant
    Dim tempVariable As Variant
    If cell.Cells.Count > 1 Then
        On Error Resume Next

        Dim singleCell As Range
        For Each singleCell In cell
            Debug.Print singleCell.Address(True, True)
            tempVariable = Empty
            tempVariable = singleCell.Validation.Type

            If IsEmpty(tempVariable) Then
                Dim cellWithoutValidationFound As Boolean
                cellWithoutValidationFound = True
            Else
                Dim cellWithValidationFound As Boolean
                cellWithValidationFound = True
            End If

            If cellWithValidationFound And cellWithoutValidationFound Then
                On Error GoTo 0
                HasValidation = Null
                Exit Function
            End If
        Next

        HasValidation = cellWithValidationFound
    Else
        tempVariable = Null
        tempVariable = cell.Validation.Type
        HasValidation = IsNull(tempVariable)
    End If
End Function

I'd recommend caution re: using On Error statements in utility functions like this. Any custom error handling in the main procedure (ie "On Error GoTo lineNum") will be overwritten by this function, so you'll have to remember to manually change it back any time you use the function in a procedure with custom error handling.

My advice would be to pick a personal rule-- either "Never use On Error statements in utility functions, only in main procedures" or "Only use On Error statements in utility functions, never in main procedures"-- and stick to it.

If you do use them, make sure that you switch back to On Error GoTo 0 for all exits from the function. In the first portion of the "IF" block in your current code, you only switch back if the function returns null:

On Error Resume Next
Dim singleCell As Range
For Each singleCell In cell
    '...do stuff, then

    If cellWithValidationFound And cellWithoutValidationFound Then
        On Error GoTo 0
        HasValidation = Null
        Exit Function
    End If
Next

'On Error GoTo 0 needs to be added here
HasValidation = cellWithValidationFound

In this case, it's possible to avoid the "On Error" statements entirely by using the SpecialCells and Intersect methods. Only caveat is that it assumes that your range's parent worksheet contains at least one blank cell:

Function hasValidation(rng As Range) As Variant

    'Find first empty cell in ws and add validation
    'This ensures that at least 1 cell in ws contains validation
    Dim emptyCell As Range
    Set emptyCell = rng.Parent.Cells.SpecialCells(xlCellTypeBlanks).Cells(1)
    emptyCell.Validation.Add Type:=xlValidateList, Formula1:="1"

    'Get range of all cells in ws with validation, then remove validation from empty cell
    Dim validationCells As Range
    Set validationCells = rng.Parent.Cells.SpecialCells(xlCellTypeAllValidation)
    emptyCell.Validation.Delete

    'Get intersection of validation cells and range being tested
    Dim rngUnion As Range
    Set rngUnion = Intersect(rng, validationCells)

    'Determine whether entire/partial range has validation and return value
    If rngUnion Is Nothing Then
        hasValidation = False
    ElseIf rngUnion.Count = rng.Count Then
        hasValidation = True
    Else
        hasValidation = Null
    End If

End Function

It'd be possible to design a slightly safer version that accounts for sheets with no blank cells, but that scenario is so unlikely that it's probably not worth the effort.

EDIT: Actually, it turns out that Range.SpecialCells(xlCellTypeBlanks) is way worse than I thought. Not only is it obscenely slow for large ranges, it also only searches the Used Range, not the entire sheet. So you may have no option but to use error handling.

Luckily, you can use the same basic format while incorporating On Error. Since it uses Intersect instead of looping through cells, it should be faster than your version. Also, rather than returning Null in mixed cases, it uses optional parameter that alters how the function treats ranges with a mix of validation/no validation. That's just a stylistic preference, I'm not a big fan of Null. Here's my final version:

Function hasValidation(rng As Range, Optional entireRange As Boolean = False) As Boolean

    'Get range of all cells in sheet containing validation
    On Error Resume Next
    Dim validationCells As Range
    Set validationCells = rng.Parent.Cells.SpecialCells(xlCellTypeAllValidation)
    On Error GoTo 0

    'If no cells contain validation, return False and exit
    If validationCells Is Nothing Then
        hasValidation = False
        Exit Function
    End If

    'Get intersection of validation cells and range being tested
    Dim rngUnion As Range
    Set rngUnion = Intersect(rng, validationCells)

    'Determine whether entire/partial range has validation and return value
    If rngUnion Is Nothing Then
        hasValidation = False
    Else
        hasValidation = IIf(rngUnion.Count = rng.Count, True, Not entireRange)
    End If

End Function


Laravel controller Single Responsibility principle I have a method in Laravel controller. How can I improve this code? How can I implement single responsibilty here or maybe there are some other tips I can use in this code? I know that controller methods should be kept as small as possible,but I don't know if it's better idea to split this code in separate methods.

public function displayData(CityRequest $request)
    {
        if (isset($request->validator) && $request->validator->fails()) {
            return redirect()->back()->withErrors($request->validator->messages());
        }
        else {
            $cityName = $request->city;
            $cityName = str_replace(' ','-',$cityName);
            $cityName = iconv('UTF-8', 'ISO-8859-1//TRANSLIT//IGNORE', $cityName);
            $response = $this->guzzle->request('GET', asset("/api/products/recommended/" . $cityName));
            $response_body = json_decode($response->getBody());
            if(isset($response_body->error))
            {
                return redirect()->back()->withErrors(['city'=>$response_body->error]);
            }
            $city = $response_body->city;
            $condition = $response_body->current_weather;
            $products = $response_body->recommended_products;
            return back()->with(['city' => $city, 'condition' => $condition, 'products' => $products]);
        }
    }

Here are some simple tips on how you can improve your code. There are still more you can do to improve it, but is a good start and it helps you get closer to the single responsibility principle.

Move the API request to a service (App\Services\ProductsApi\Client).
Move the formatting of city to a helper (App\Helpers\Format).
Consider moving the formatting of city to CityRequest so the controller doesn't have to do it.
No need for new variables, use the properties returned by $response instead.
No need for else-statement, since the if-statement has return.

Your controller:

use App\Services\ProductsApi\Client;

public function __construct(Client $client)
{
    $this->client = $client;
}

public function displayData(CityRequest $request)
{
    if (isset($request->validator) && $request->validator->fails()) {
        return redirect()->back()->withErrors($request->validator->messages());
    }

    $response = $this->client->getRecommended(
        App\Helpers\Format::slugify($request->city)
    );

    if ($response->error) {
        return redirect()->back()->withErrors([
            'city' => $response->error
        ]);
    }

    return back()->with([
        'city' => $response->city ?? null,
        'condition' => $response->current_weather ?? null,
        'products' => $response->recommended_products ?? null,
    ]);
}

Helper:

public static function slugify($value)
{
    return Illuminate\Support\Str::slug(
        iconv('UTF-8', 'ISO-8859-1//TRANSLIT//IGNORE', $value),
        '-'
    );
}

(None of the code above is tested)


3 sum implementation in python 2.7 Working on below problem, looking for advice on code style, functional bug and time complexity improvements.

Also looking for advice in,

If array is not sorted, any solutions available for O(n^2) time complexity?
In my loop, I always search from exclude_index + 1 to reduce duplicate, is it a safe start boundary?

Problem,

Given a sorted array and a target number, tell whether there are three numbers in the array which add up to the target number. (Time complexity should be O(n^2))

Source code,

def two_sum(numbers, start, end, target, exclude_index):
    while start < end:
        if start == exclude_index:
            start += 1
        elif end == exclude_index:
            end -= 1
        elif numbers[start] + numbers[end] > target:
            end -= 1
        elif numbers[start] + numbers[end] == target:
            print numbers[start], numbers[end], numbers[exclude_index]
            start += 1
            end -= 1
            while start < end and (numbers[start] == numbers[start - 1] or start == exclude_index):
                start += 1
            while start < end and (numbers[end] == numbers[end + 1] or end == exclude_index):
                end -= 1
        else:
            start += 1
def three_sum(numbers, target):
    for exclude_index in range(len(numbers)):
        if exclude_index > 0 and numbers[exclude_index] == numbers[exclude_index - 1]:
            continue
        two_sum(numbers, exclude_index + 1, len(numbers) - 1, target - numbers[exclude_index], exclude_index)

if __name__ == "__main__":
    numbers = [1, 3, 4, 6, 7, 8, 9, 9, 10, 12]
    three_sum(numbers, 15)

As with most of your questions, your code violates PEP8.

Don't print your output.
Since \$\text{start} = \text{exclude_index} + 1\$ you can never visit exclude_index.

Your code with the above changes make your code more readable.
But it'd be better if you:

Start using Python's standard library. Itertools can drastically improve your codes readability.
Stop relying on while so much.
Use a dictionary, it's fast, and increases readability.
sorted runs in \$O(n\log(n))\$ time.

Using itertools.combinations_with_replacement and collections.Counter you can generate all three of your numbers, still in \$O(n^2)\$ time.

Pass a sorted list to combinations_with_replacement, and generate two numbers.
Check target - i - j is in the list of numbers, and that it is greater or equal to i and j.
Group; i, j and k, in a Counter and check there is enough of each number in the original list.

import collections
import itertools

def three_sum(numbers, target):
    Counter = collections.Counter
    count = Counter(numbers)
    for i, j in itertools.combinations_with_replacement(list(sorted(count)), 2):
        k = target - i - j
        if k in count and j <= k >= i:
            nums = (i, j, k)
            if all(count[k] >= v for k, v in Counter(nums).items()):
                yield nums


Saving a column from a file to a list I want to read only 1 column from file and save it in the list column. I have this code and I don't know how to optimize it. It takes lots of time because I need to call it a lot of times.

column = (sum(1 for line in open(file) if line.strip("\n")))*[0]
counter_row = 0
counter_column = 0
with open(file, 'r') as f:
    for row in f:
        row = row.strip("\n")
        if row:
            for el_column in row.split(","):
                if counter_column==n_column:
                     column[counter_row]=el_column
                counter_column = counter_column + 1
        counter_row = counter_row + 1
        counter_column = 0

Your biggest problem is the first line:

column = (sum(1 for line in open(file) if line.strip("\n")))*[0]

Opening the file twice is a bad idea for performance, probably much worse than not knowing how large of a list to allocate.  Furthermore, that statement only allocates one element per non-empty line, whereas the counter_row counts every line in the file.  So, the code would crash with an IndexError if there are any empty lines.

The inner for loop is just a complicated way to do indexing.

Assuming that you want empty lines to be represented by 0, you could write:

column = []
with open(file) as f:
    for line in f:
        row = line.strip("\n")
        column.append(row.split(",")[n_column] if row else 0)

If you don't need to test for empty lines, then you could just write:

with open(file) as f:
    column = [line.strip("\n").split(",")[n_column] for line in f]

You should consider using the csv module, though, which can handle CSV quoting properly.


Optimizing 'yes' UNIX command in Java the One of Unix core commands is yes which simply prints infinity number of letter y.

I read a blog post (https://matthias-endler.de/2017/yes/) about how people optimize it to output y with a speed of 10 GB/s. I get 7.5 GB/s on my machine using this command.

I tried to achieve similar results with Java but it stopped at 70 MB/s.

My code is:

public class yes {

    static byte[] bb = new byte[]{'y','\n'};

    public static void main(String[] args) throws IOException {

        BufferedOutputStream writer = new BufferedOutputStream(System.out);
        for (;;) {
            writer.write(bb);
        }

    }
}

Do you have any idea how to optimize it to get results similar to UNIX version?

Your program calls the write() method of the BufferedOutputStream
with a two byte array. These two bytes are stored in the
stream's buffer, and the buffer is written to standard output when it is
full.

On macOS 10.13 the fs_usage tool shows that the buffer size is 8 kB, and on my MacBook the

  java -cp . yes | pv > /dev/null

benchmark reports that the data is written with about 50 MB/s.

This can be improved by calling write() with a larger byte array:

public class yes {

    static final int BUFFERSIZE = 8 * 1024;
    static byte[] bb = new byte[BUFFERSIZE];

    public static void main(String[] args) throws IOException {

        for (int i = 0; i < BUFFERSIZE; i += 2) {
            bb[i] = 'y';
            bb[i+1] = '\n';
        }

        BufferedOutputStream writer = new BufferedOutputStream(System.out);
        for (;;) {
            writer.write(bb);
        }
    }
}

Now only one write() call is needed to write 8 kB data, instead
of 4096 calls. On my MacBook this increased the speed to
about 2 GB/s, that is a factor of more than 40.

This can possibly be further improved by choosing a larger buffer.

With a byte array larger that the stream buffer size, BufferedOutputStream does
not buffer anymore and writes the data to stdout immediately. Therefore the
"lower-level" unbuffered FileOutputStream would be sufficient:

    FileOutputStream writer = new FileOutputStream(FileDescriptor.out);
    for (;;) {
        writer.write(bb);
    }

However, I could not observe a difference in the performance.


CROSS JOIN solution for records "missing" from a link table Use case: We have a simple two tables plus a link table setup using some modern RDBMS.

For example:

Hospital table (id, name, ...)

Physician table (id, name, ...)

Visiting_Privilege table (hospital_id, physician_id, ...)

Our goal is to find records that are "missing" (absent) from the link table.

Using our example: List physicians and hospitals where they are not authorized to see patients.

The proposed solution: Create a full Cartesian Product between the two main tables,  JOIN it with the link table, filter records with either of the IDs as NULL.

SELECT p.name, h.name
FROM Physicians p
CROSS JOIN Hospitals h
LEFT OUTER JOIN Visiting_Privilege v ON p.id = v.physician_id AND h.id = v.hospital_id
WHERE v.physician_id IS NULL OR v.hospital_id IS NULL;

We assume that tables are properly indexed and the SQL engine provides reasonable query optimization.

Would it be a right way to attack the problem?
Is there a better solution that avoids the cost of a full Cartesian Product?

I am pretty sure that this is the right approach to solve this problem, but depending on the RDBMS, it is not necessary the use of any index to access each row.
Oracle, for instance, will probably operate with Hash Joins in this case, because of the high  cardinality of the query (at the end of this task all rows of this 3 tables must be accessed) and if you have an index on the link table, it will use an Index Fast Full Scan.
I don't know if other RDBMS have the power of Hash Joins operations, which uses more memory instead of IOs.

Another syntax that could be used in this problem is the NOT EXISTS operator. Choose which syntax you think will translate more clearly the meaning of the query. In Oracle it will forces to perform an Hash Join Anti operation in the last table.