jQuery conditional statement to check if a select option has either of 4 values selected

// You can use the document.ready as both encapsulation as well as
// to make sure the DOM is ready for operation.
$(function ($) {

  // Unless these are dynamic (the set could change), it's better to cache them
  var optionBox = $('.optionBox');
  var dropOff = $('#dropOff');
  var pickUp = $('#pickUp');
  var field1 = $('#field_1');

  field.show();

  // Your select box has an ID. They are faster than attribute selectors
  // in both parsing ase well as fetching from the DOM
  $('#serviceChoice').change(function () {

    // The current context of this function is the DOM element, which is the select
    // You can use it to get the value, rather than using a selector again. Removes
    // the need for jQuery to parse the selector and look for it in the DOM
    switch ($(this).val()) {
    case 'A':
      optionBox.hide();
      dropOff.show();
      break;
    case 'B':
    case 'C':
    case 'D':
      optionBox.hide();
      pickUp.show();
      break;
    default:
      optionBox.show();
    }
  });
});

Minus the comments, here's what you get:

$(function ($) {

  var optionBox = $('.optionBox');
  var dropOff = $('#dropOff');
  var pickUp = $('#pickUp');
  var field1 = $('#field_1');

  field.show();

  $('#serviceChoice').change(function () {

    switch ($(this).val()) {
    case 'A':
      optionBox.hide();
      dropOff.show();
      break;
    case 'B':
    case 'C':
    case 'D':
      optionBox.hide();
      pickUp.show();
      break;
    default:
      optionBox.show()
    }
  })
});


Separation of code into functions and use of patern matching in Haskell

To me, serialize especially seems pretty ugly; the use of the where clauses and the similarity of the latter two pattern match cases all seems bad to me, yet I'm unsure what should be done instead.

You can rewrite the serialize function using as-patterns which will eliminate the where clause like so.

serialize :: Atom -> Word8
serialize TerminationAtom = 0
serialize all@(NonOffsetAtom _ _ _) = fromIntegral $ (getAtomType all) `shiftL` 6 .|.
                                                     (getFillField all) `shiftL` 4 .|.
                                                     (getDataField all)
serialize all@(OffsetAtom _ _ _)    = fromIntegral $ (getAtomType all) `shiftL` 6 .|.
                                                     (getFillField all) `shiftL` 3 .|.
                                                     (getDataField all)

You could eliminate the duplication further by extracting the body of the function into another function parameterized over the shift value. Something like this.

serialize all@(NonOffsetAtom _ _ _) = shifty 4 all
serialize all@(OffsetAtom _ _ _)    = shifty 3 all

shifty n a = fromIntegral $ (getAtomType a) `shiftL` 6 .|.
                            (getFillField a) `shiftL` n .|.
                            (getDataField a)

  Should I use Word8 as the return type of all the functions (the return values are all small enough to fit)?

Yes. If your functions logically only operate on 8-bit byte values, then that's an important fact you can and should capture in the type signature. If returning a value of 256 or 500000 or -1 would be absurd, then eliminate the possibility of that even happening!

In General

There are other odds and ends that could be changed or cleaned up, like comparing Bool arguments to True or False instead of using them directly (getAtomType) or using ifs instead of more pattern matching (getFillField (NonOffsetAtom _ True _) = ...).

Update

Having slept on it and cleared my head, I think your code would be much improved by leveraging ADTs and a serialization typeclass. As it stands your solution feels very C-like, and your data types are used like structs, arrays and low-level machine ints instead of rich data. I'll sketch an outline of what I would do here, please feel welcome to interrogate me in the comments if the full picture doesn't come through for you.

data Four = Zero | One | Two | Three

data AtomType = ShortBase   Four | LongBase
              | ShortOffset Bool | LongOffset

newtype FillType = Fill Four

data DataField = ...

data Word = Terminal
          | Word AtomType FillType DataField

class Serialize a where
    serialize :: a -> Word8

instance Serialize Four where
    serialize Zero  = 0
    serialize One   = 1
    serialize Two   = 2
    serialize Three = 3

instance Serialize AtomType where
    serialize (ShortBase n)       = serialize n `shiftL` 6
    serialize LongBase            = 4 `shiftL` 6
    serialize (ShortOffset False) = 5 `shiftL` 6
    serialize (ShortOffset True)  = 7 `shiftL` 6
    serialize LongOffset          = 6 `shiftL` 6

...

instance Serialize Word where
    serialize Terminal     = 0
    serialize (Word t f d) = serialize t .|. serialize f .|. serialize d

My goal here was to make everything as correct-by-construction as possible (q.v. the Four type) and to reduce the opportunities for ambiguity (e.g., a "Fill Field" may mean any number of things if all you're looking at is a literal 0, so the representation should be as precise as necessary and no more).

On the benefits of this approach

If you can determine the names based on the problem domain, this will be extremely readable
Requires less dead code (In particular I'm looking at the support for TerminationAtom everywhere)
Easily made symmetrical with a Deserialize class

Intermediary representations of data can always slow down your code, but with aggressive inlining I don't think this would be too much of a performance hit.


You have an unneeded comma in line 5. Although it won't make Node.js crash it is invalid JavaScript. And as you are using semicolons you should use them everywhere. So you should end your statement in line 7 with a semicolon.


Using ID's with a "scope" -like hierarchy

Preface: these are just my personal opinions based on experience.

Don't force the user of the class to remember to do stuff

The setContext(const char*) method is a method which the user of the class is required to call to have proper operation. As such the user is at risk of forgetting to call this method and thus introduce a bug in the software. Further more the user must remember to be consistent in spelling of the context name or face the risk of introducing subtle bugs where different contexts are used by mistake.

If you wish to go with the concept of contexts (say that 10 time quickly), I would make a "context" a first class object which is required for creating instances of resources. Like this:

class ResourceContext{
public:
    template<typename... Arg>
    std::shared_ptr<Resource> make(Arg... args);
};

class ResourceManager{
public:
    std::shared_ptr<ResourceContext> createContext(const char*);
    std::shared_ptr<ResourceContext> findContext(const char*);
private:
    friend class ResourceContext;
};

ResourceManager mgr;
auto gameContext = mgr.createContext("Game");
Sprite sprite(gameContext, "tex/tex1.png");

This approach solves my two worries

You can not forget to change context. You are forced to find the correct context.
You can not silently introduce a typo, either you explicitly create a new context or you look up an existing one (with an error if it doesn't exist).

Lookup by reference when possible

If your texture/sprite class is dependent on the window they are displayed in (as opposed to the SDL context) then it is quite reasonable to expect them to be created from a context where the window is available. Why not simply pass an instance of the resource<Window> to the constructor of the sprite/texture instead of doing a lookup by string ID?

Your current approach implicitly reserves "magic" names for specific instances of specific classes the risk here is that an unknowing user may create some kind of conflict because they didn't know of all the reserved names in your system.

One size doesn't fit all (Stop sugar coating my globals)

In general I believe that this kind of resource manager should only be used for obtaining external resources such as textures, sounds etc. To me it looks like you're trying to sugar coat having global variables, because that's essentially what this is being used like in your example. Using it for SDL context is overkill, why would you want to shutdown SDL during any point of your program's lifetime? What is the point of having it managed as a resource over just a simple global?

In all honesty, I think you are over-engineering this.


Nested layouts - BoxLayout inside BorderLayout

When you create a form, a GridBagLayout is usually needed.

Here's one way to create the form in your question.

import java.awt.GridBagConstraints;
import java.awt.GridBagLayout;
import java.awt.Insets;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.SwingUtilities;

public class SimpleGridBagLayout implements Runnable {

    @Override
    public void run() {
        JFrame frame = new JFrame("GridBagLayout Test");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        JPanel panel = new JPanel();
        panel.setLayout(new GridBagLayout());

        GridBagConstraints gbc = new GridBagConstraints();
        gbc.insets = new Insets(10, 10, 10, 10);
        gbc.gridwidth = 2;
        gbc.gridx = 0;
        gbc.gridy = 0;

        JLabel titleLabel = new JLabel("Control Panel");
        panel.add(titleLabel, gbc);

        gbc.anchor = GridBagConstraints.LINE_START;
        gbc.fill = GridBagConstraints.HORIZONTAL;
        gbc.gridwidth = 1;
        gbc.gridy++;

        JLabel fromDateLabel = new JLabel("From date:");
        panel.add(fromDateLabel, gbc);

        gbc.gridx++;

        JButton fromDateButton = new JButton("...");
        panel.add(fromDateButton, gbc);

        gbc.gridx = 0;
        gbc.gridy++;

        JLabel toDateLabel = new JLabel("To date:");
        panel.add(toDateLabel, gbc);

        gbc.gridx++;

        JButton toDateButton = new JButton("...");
        panel.add(toDateButton, gbc);

        frame.add(panel);
        frame.setSize(250, 200);
        frame.setVisible(true);
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new SimpleGridBagLayout());
    }

}


jQuery multiple events execute same function refactoring

Your code isn't all that bad. However because you are binding key events on the document, you need to allow room, for other key events to be bound, for other tasks, so your code doesn't interfere with other ESC functions.

The concept is that you bind on click, and unbind upon hiding your modal. This way the key events are only applicable when your code is run.

// save your doc once, it's faster
var doc = $(document);
    // bind a function, which you can unbind later, so esc can be used in other cases as well, without triggering your function
    doc.bind('keydown', _processKey);

// on click binding for as many elements as you like.
// The preventDefault shouldn't influence other elements in this case.
$("#modal, #modal-object-container > a").click(function(ev) {
    ev.preventDefault();
    hideModal();
});

// when you hide your modal, you unbind the event, so the document is free for other events and functions
function hideModal() {
    doc.unbind('keydown', _processKey);
}

// have a 'private' function to do what you want. This way you can unbind it anytime you like
function _processKey(ev) {
    if (ev.keyCode == '27') {
        hideModal();
    }
}


DDOS Mechanism: Improving thread safety & loop speed?

private bool _cycleActive;

You actually don't need this variable. The Task class has a Status property.

Thread.Sleep(2);

And the async/await classic one (like the classic namespace std in c++). You don't want to block your thread with that. Instead, you should use Task.Delay and make your code async

public void Stop()
{
    _cycleActive = false;
}

For stopping you should use the CancellationTokenSource and the CancellationToken. See Task Cancellation.


Sorting Each Entry (code review + optimization)

There are many improvements that can be done, but here are a few things I identified:

I wouldn't have the require call within the function itself. I would rather create a top-level module with all listed dependencies. The updateBottomBar function would be defined in this module:

require(["dojox/collections/ArrayList"/*, other deps...*/], function(ArrayList) {
    function updateBottomBar() {
        //...
    }
});

Have a look at https://dojotoolkit.org/documentation/tutorials/1.8/modules/

It's very inefficient to merge the multiple arrays every-time. You should rather maintain an additional array that stays in sync with the others. When a new item is pushed in one of the other arrays, you reflect the changes in that additional array directly (same logic with removal). However, if you can, I would rather have a single collection to maintain and use a discriminator such as a category property to categorize items, so instead of holding strings, the collection would contain { country: String, category: String } objects.
With your current implementation, you also throw away some useful information that you need to recompute later. For example if(selectedCountries.contains(country)) shouldn't be necessary since you had that knowledge when you pushed the items in the bottomBar list. Instead of pushing a simple string, push an object like proposed in point #2. Then you will be able to check if the items belongs to the selected category in O(1) time by doing something like:

switch (country.category) {
    'selected': //do something break;
}

Concatenating multiple strings with the + operator isin't the most efficient way. Instead you should push your strings in an array and then use theArray.join('') to perform the concatenation.

Also note that you can create DOM elements dynamically with document.createElement and make the process efficient by using document fragments (document.createDocumentFragment).
You have a typo -> var initalContent="Countires: <label>";
initalContent != ... Did you mean += ?


Definition of balanced

The usual definition of a balanced binary tree:

The left and right subtrees' heights differ by at most one, AND
The left subtree is balanced, AND
The right subtree is balanced

Your algorithm only checks the level of the root, comparing the height of the left and right branches of the root. It doesn't go deeper, so the left or right subtrees may be unbalanced.

Unnecessary conditions

This condition can be simplified:

if (root == null || (root != null && root.left == null && root.right == null)) {

You can drop the root != null, because it will always be true.
This is exactly the same:

if (root == null || (root.left == null && root.right == null)) {

This chain of conditions has the same issue,
with some unnecessary conditions that can be eliminated:

if (root == null) {
    return 0;
} else if (root.left == null && root.right == null) {
    return 1;
} else if (root.left == null && root.right != null) {
    return 1 + findHeight(root.right);
} else if (root.left != null && root.right == null) {
    return 1 + findHeight(root.left);
} else {

Can be simplified to:

if (root == null) {
    return 0;
} else if (root.left == null && root.right == null) {
    return 1;
} else if (root.left == null) {
    return 1 + findHeight(root.right);
} else if (root.right == null) {
    return 1 + findHeight(root.left);
} else {

Using boolean expressions

You can use boolean expressions directly. For example like this:

return Math.abs(balancedLeft - balancedRight) <= 1;

Other simplifications

Instead of this:

int balancedRight = 0;
int balancedLeft = 0;
if (root.left != null) {
    balancedLeft = findHeight(root.left);
}
if (root.right != null) {
    balancedRight = findHeight(root.right);
}

Since findHeight immediately does a check for the case of root == null,
you could simplify the above as:

int balancedRight = findHeight(root.left);
int balancedLeft = findHeight(root.right);

Another thing, instead of adding a constant in both sides of a Math.max:

return Math.max(1+findHeight(root.left), 1+findHeight(root.right));

You could do just once:

return 1 + Math.max(findHeight(root.left), findHeight(root.right));

On closer look, you could simplify the entire findHeight method to this:

public int findHeight(TreeNode root) {
    if (root == null) {
        return 0;
    }
    return 1 + Math.max(findHeight(root.left), findHeight(root.right));
}

Naming

Instead balancedLeft and balancedRight,
leftHeight and rightHeight would have made a lot more sense.


Rock Paper Scissors in Java (I just want to know whatever feedback you have :) Thank you!)

I have some suggestions for you.

Next time, before submitting code, try to format it :)

Suggestions

Unused variable

The variable rd is not used, you can remove it.

Put variables that don’t change out of the loop

In the while loop, the rock, paper and scissors get their value reassigned with the same value each time the loop restart, move them outside of the loop.

I suggest that you put those as class constants, so you will be able to reuse the in other methods.

public class RockPaperScissors {
    public static final int ROCK = 1;
    public static final int PAPER = 2;
    public static final int SCISSORS = 3;
}

Validate the invalid choice first

In your code, you check if the chosen option is valid at the end of the loop; it will be more efficient if you check the invalid options first. This will prevent to check every other conditions and quit the loop faster.

System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
int player1 = kb.nextInt();

if (player1 != rock && player1 != paper && player1 != scissors) {
   System.out.print("Invalid response");
   break;
}

Extract some of the logic into methods

In your code, you can move some of the logic into method to allow the code to be shorter and easier to read.

I suggest make methods to check if the answer is a rock, paper or scissors; this will allow the conditions to be shorter.

public static void main(String[] args) {
   while (true) {
      //[...]
      if (isRock(player1) && isRock(random)) {
         //[...]
      }
      //[...]
   }
}

private static boolean isRock(int answer) {
   return answer == ROCK;
}

private static boolean isPaper(int answer) {
   return answer == PAPER;
}

private static boolean isScissor(int answer) {
   return answer == SCISSORS;
}

I suggest that you extract the logic that read the value in a method.

public static void main(String[] args) {
   while (true) {
      //[...]
      int player1 = getPlayer1Answer(kb);
   }
}

private static int getPlayer1Answer(Scanner kb) {
   System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
   return kb.nextInt();
}

Bonus

While you are in the method, I suggest to make another loop to verify the validity of the answer; this will make sure the provided choice is valid and ask again if not.

private static int getPlayer1Answer(Scanner scanner) {
   int value;

   while (true) {
      System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
      value = scanner.nextInt();

      if (isRock(value) || isPaper(value) || isScissor(value)) {
         break;
      } else {
         System.out.println("Invalid response");
      }
   }

   return value;
}

I suggest that you extract the logic that generates the computer answer in a method.

    private static int getComputerAnswer() {
        int[] conditions = {ROCK, PAPER, SCISSORS};
        return conditions[new Random().nextInt(conditions.length)];
    }

Refactored code

public class RockPaperScissors {
    public static final int ROCK = 1;
    public static final int PAPER = 2;
    public static final int SCISSORS = 3;

    public static void main(String[] args) {
        Scanner kb = new Scanner(System.in);
        System.out.println("Welcome to Rock Paper Scissors!");

        while (true) {
            int random = getComputerAnswer();
            int player1 = getPlayer1Answer(kb);

            if (isRock(player1) && isRock(random)) {
                System.out.print("You have tied, computer rock, please try again\n");
            } else if (isPaper(player1) && isPaper(random)) {
                System.out.print("You have tied, computer chose paper, please try again");
            } else if (isScissor(player1) && isScissor(random)) {
                System.out.print("You have tied, computer chose scissors, please try again");
            }

            if (isRock(player1) && isPaper(random)) {
                System.out.print("Computer chose paper, computer has won");
                break;
            } else if (isPaper(player1) && isRock(random)) {
                System.out.print("You win!, computer chose rock");
                break;
            } else if (isPaper(player1) && isScissor(random)) {
                System.out.print("Computer chose scissors, computer has won");
                break;
            } else if (isScissor(player1) && isPaper(random)) {
                System.out.print("You win, computer chose paper");
                break;
            } else if (isRock(player1) && isScissor(random)) {
                System.out.print("You win!, computer chose scissors");
                break;
            } else if (isScissor(player1) && isRock(random)) {
                System.out.print("Computer chose rock, computer has won");
                break;
            }
        }
    }

    private static boolean isRock(int answer) {
        return answer == ROCK;
    }

    private static boolean isPaper(int answer) {
        return answer == PAPER;
    }

    private static boolean isScissor(int answer) {
        return answer == SCISSORS;
    }

    private static int getComputerAnswer() {
        int[] conditions = {ROCK, PAPER, SCISSORS};
        return conditions[new Random().nextInt(conditions.length)];
    }

    private static int getPlayer1Answer(Scanner scanner) {
        int value;

        while(true) {
            System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
            value = scanner.nextInt();

            if(isRock(value) || isPaper(value) || isScissor(value)) {
                break;
            } else {
                System.out.println("Invalid response");
            }
        }

        return value;
    }
}
```


Find a better logic to remove duplicated code from two different methods

Short version: Use a parameter!

public function handleOperation($action) {}
public function register() { $this->handleOperation("register"); }
public function unregister() { $this->handleOperation("unregister"); }

Slightly longer version: Don't be a smartass

You don't need to dynamically call spl_autoload_$function, it's pointless and unreadable. Just call spl_autoload_registerand spl_autoload_unregisterwhen you need it. It's unlikely for the function names to change.


Python web-scraper to download table of transistor counts from Wikipedia

Is there a way to simplify this code?

Yes. Don't scrape Wikipedia. Your first thought before "should I need to scrape this thing?" should be "Is there an API that can give me the data I want?" In this case, there super is.

There are many informative links such as this StackOverflow question, but in the end reading the API documentation really is the right thing to do. This should get you started:

from pprint import pprint
import requests, wikitextparser

r = requests.get(
    'https://en.wikipedia.org/w/api.php',
    params={
        'action': 'query',
        'titles': 'Transistor_count',
        'prop': 'revisions',
        'rvprop': 'content',
        'format': 'json',
    }
)
r.raise_for_status()
pages = r.json()['query']['pages']
body = next(iter(pages.values()))['revisions'][0]['*']
doc = wikitextparser.parse(body)
print(f'{len(doc.tables)} tables retrieved')

pprint(doc.tables[0].data())

This may seem more roundabout than scraping the page, but API access gets you structured data, which bypasses an HTML rendering step that you shouldn't have to deal with. This structured data is the actual source of the article and is more reliable.


Similar methods to update today's sales, revenue, and profit

Instead of dynamically defining a bunch of methods, you could just have one method that takes an argument that tells it what kind of work to do:

def self.create_or_update_today(kind, network)
  meth = :"#{kind}_today"
  total = Conversion.send(meth, network)

  aggregate_name = "#{kind}Today"

  today = find_or_initialize_by(agregateName: aggregate_name, networkName: network)
  today.update(date: Date.today, value: total)
end

You would call it like this:

Statistic.create_or_update_today(:sales, network)

(You'll notice I used find_or_initialize_by instead of find_or_create_by. This way the code will always perform two queries instead of performing three if it's a new record. update will create the record of it doesn't exist.)

If you wanted you could still define individual methods, but this way it's much cleaner:

def self.sales_today(network)
  self.create_or_update_today(:sales, network)
end

def self.revenue_today(network)
  self.create_or_update_today(:revenue, network)
end

# ...

You could also do this with define_method in a loop as Devon does, but it would save very few lines while sacrificing a lot of readability and, as britishtea points out, introducing documentation issues.

P.S. Just a tip. Don't use size for this:

where('DATE(created_at) = ?  AND networkName = ?',
      Date.today, network).size

You only want to know how many records there are, but when you do this it fetches all of the data in all of the records and initializes a Conversion object for each, which is really wasteful. Instead of size, use Rails' built-in count method:

where('DATE(created_at) = ?  AND networkName = ?',
      Date.today, network).count

This will let the database perform the calculation so ActiveRecord only needs to fetch a single number.


Making the save and cancel buttons editable

How about using just one declaration and an if block? You remove the top three lines by this for each function -

assuming buttons are <button>

var originalTexts = {};
$("div.edit-panel").find("button,span").click(function(){
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var id = $(this).parent().attr("id");
    var currentText = text.text();

    if ($(this).is("span")){
        panel.find(".edit-panel").addClass('editable');
        text.addClass("content-editable").prop('contenteditable', 'true');
        originalTexts[id] = text.text();
    }else if ($(this).hasClass("cancel-editable")){
        text.text(originalTexts[id]);
        panel.find(".edit-panel").removeClass('editable');
        text.removeClass("content-editable").prop('contenteditable', 'false');
    }else if ($(this).hasClass("save-editable")){
        text.text(currentText);
        panel.find(".edit-panel").removeClass('editable');
        text.removeClass("content-editable").prop('contenteditable', 'false');
    }
});

You can reduce more codes if you can store the id and other information inside the data attributes of each button. But I don't know the html, so could not suggest on that.


something unpythonic?

Consider using plural identifiers: test_data_tuples, test_data_elts. Later, for example, you use a perfectly lovely for elt in elts: idiom.

In Element __init__, I wouldn't mind seeing cls='unknown' spelled out, but I am really missing a docstring that explains what the heck idf stands for. Also, as near as I can tell, data really is a coord, e.g. (x, y).

Kudos on k = nb_classes. I feel you did a good job of making the public API very clear, and then within the body k is conventional.

I can't say I'm a fan of import random as rd. It needlessly obscured the fairly obvious meaning of rd.choice(elts).data.

The for itr ... loop exhibits admirable clarity.

Testing if len(elts_in) == 0:, plus the map, seems like it's maybe a bit clunky. I'm just not understanding why empty is a special case, perhaps we could use sum and lambda, or a helper function? It seems like we could always find something sensible to assign to c.data, even in the empty case. Or maybe what I'm really looking for is an if that rapidly moves on to a continue.

The print('\n'+str(c)) is not very pythonic. I'd prefer print('\n%s' % c). Ok, that's the last of the tiny nits I will pick.

Overall, it looks like fairly solid code.

  changes could drastically improve the code's performance?

Sorry, don't see anything obvious that would help.


Downloading stock information from Yahoo! Finance

I don't know what would cause the large difference in time, but I found a couple minor factors:

Application.ScreenUpdating = False is performed in each loop (~4 000 times I assume)
Application.DisplayAlerts = False is not only performed in the same volume but also not negated in the scope of this question.
The lack of .Value2 has a potential performance demerit as discussed here.


Your prime testing code is wrong. For example, it would consider 14 to be a prime! You cannot take a shortcut by only testing divisors > 13. Instead:

static bool IsPrime(Int64 p)
{
    if (p % 2 == 0) return false;
    Int64 max = (Int64) Math.Ceiling(Math.Sqrt(p));
    for (Int64 divisor = 3; divisor < max; divisor += 2)
    {
        if (p % divisor == 0) return false;
    }
    return true;
}

Points to note:

I use a for loop instead of while, as this better expresses what I'm doing.
I only go up to divisor < ceiling(sqrt(p)), which saves one iteration. Not significant, but it's less redundant this way.
While I do test for p % 2 == 0, this branch will never be taken because we will later modify our prime number search loop to only search odd numbers. I leave that test here for correctness.
Note also that smaller numbers are more likely to be divisors than larger numbers. It's therefore advantageous to test them early.

Your prime search loop looks at each number in from 13 onward. However with the exception of 2, only odd numbers can be primes. We can therefore halve the search time by incrementing in steps of two.

int count = 6;
int targetCount = 10001;
long x;
for (x = 13 + 2; count < targetCount; x += 2)
{
    if (IsPrime(x)) count++;
}

But wait! If we are finding all prime numbers from 13 onward we can use those to speed up our prime test by only testing appropriate prime factors! We can do this by creating a table of primes:

long[] primes = new long[targetCount];
primes[0] =  2;
primes[1] =  3;
primes[2] =  5;
primes[3] =  7;
primes[4] = 11;
primes[5] = 13;

int counter = 6;
for (long x = primes[counter - 1] + 2; counter < targetCount; x += 2)
{
    if (IsPrime(x, primes)) primes[counter++] = x;
}
// the result is in primes[targetCount - 1]

where IsPrime has been changed to

static bool IsPrime(Int64 p, long[] primes)
{
    Int64 max = (Int64) Math.Ceiling(Math.Sqrt(p));
    foreach (long divisor in primes)
    {
        if (divisor > max) break;
        if (p % divisor == 0) return false;
    }
    return true;
}

And hey presto, is this fast! I get 104743 as an answer in a matter of milliseconds.

How did I make this code so fast?

Write correct code without taking bad shortcuts.
Think about what I need and what I have anyway.
Then, figure out an elegant way to connect those two states. Here, my experience with dynamic programming reminded me to build a table of primes.


Based on the naming guidelines properties shouldn't be named using some kind of Snake_Case casing.  They should be named using PascalCase casing.

Without seeing OutboxResponse's  implementation and its other usages, I would suggest initializing the Violations property in the class itself.

The loops inside the switch cases are very similiar. They only differ by the used "prefix" e and r. These should be extracted to a separate method to reduce code duplication.

You have a lot of vertical space wasted by using to many new lines. This reduces readability.

English isn't my first language so this could be wrong, but I guess "Errors was found in packet" should be "Errors were found in packet".

These variables

string violationCode = "g:body/g:code/text()";
string violationLevel = "g:body/g:level/text()";
string violationName = "g:body/g:name/text()";

string itemResultResult = "g:body/g:itemResults/g:itemResult/g:result/text()";

aren't used and should be removed.

You should always declare variables as near as possible to their usage.

By extracting the creation of the XMLDocument to a separate method you can reduce the size of the method which adds readability.

If you need a comment to describe for what a variable stands then this variable isn't  properly named.

After applying the mentioned points the refactored method we would get

public OutboxResponse CheckSendResponseForOutboxPacket(Outbox outbox)
{
    XmlDocument responseDocument = GetXMLDocument(outbox);

    XmlNamespaceManager nsmanager = new XmlNamespaceManager(responseDocument.NameTable);
    nsmanager.AddNamespace("g", "http://google.com");

    var rootElement = responseDocument.DocumentElement;

    OutboxResponse outboxResponse = new OutboxResponse();
    outboxResponse.Violations = new List<OutboxResponseViolation>();

    string prefix;
    string xpath;
    string resultPath = "r:body/r:result/text()";

    switch (rootElement.SelectSingleNode(resultPath, nsmanager).Value)
    {
        case "success":
            outboxResponse.Result = "Packet received without errors";
            xpath = "g:body/g:itemResults/g:itemResult/g:violations/g:violation";
            prefix = "g";
            break;

        case "failure":
            outboxResponse.Result = "Errors were found in packet";
            xpath = "g:body/g:violations/g:violation";
            prefix = "r";
            break;

        default:
            return outboxResponse;

    }

    XmlNodeList nodes = rootElement.SelectNodes(xpath, nsmanager);
    outboxResponse.Violations.AddRange(GetOutputViolations(nodes, nsmanager, prefix));

    return outboxResponse;

}
private XmlDocument GetXMLDocument(Outbox outbox)
{
    var content = outbox.Outbox_content.FirstOrDefault().response_content;
    XmlDocument document = new XmlDocument();
    document.LoadXml(content);
    return document;
}
private IList<OutboxResponseViolation> GetOutputViolations(XmlNodeList nodes, XmlNamespaceManager nsmanager, string prefix)
{
    IList<OutboxResponseViolation> violations = new List<OutboxResponseViolation>();
    string levelPath = prefix + ":level/text()";
    string namePath = prefix + ":name/text()";
    string descriptionPath = prefix + ":description/text()";

    foreach (XmlNode node in nodes)
    {
        violations.Add(new OutboxResponseViolation
        {
            Level = node.SelectSingleNode(levelPath, nsmanager).Value,
            Name = node.SelectSingleNode(namePath, nsmanager).Value,
            Description = node.SelectSingleNode(descriptionPath, nsmanager).Value
        });
    }
    return violations;
}


Your function has a high complexity because of the 2 nested loops.

Here is my implementation. I'm not sure if it's more efficient, but I think it's more readable.

function findAndAppendCharacter(countArray, currentChar) {
    let foundAt = -1

    countArray.forEach((pair, i) => {
        if(pair.letter == currentChar) {
            foundAt = i
        }
    })

    if(foundAt == -1) {
        countArray.push({
            letter: currentChar,
            count: 1
        })

        return
    }

    countArray[foundAt].count++
}

function findAnagram(firstWord, secondWord) {
    firstWord = firstWord.replace(" ", "")
    secondWord = secondWord.replace(" ", "")

    if(firstWord.length != secondWord.length) {
        return "Not an anagram!"
    }

    let letterCountArray = {
        first: [],
        second: []
    }

    for(let i = 0; i < firstWord.length; i++) {
        findAndAppendCharacter(letterCountArray.first, firstWord.charAt(i).toLowerCase())
        findAndAppendCharacter(letterCountArray.second, secondWord.charAt(i).toLowerCase())
    }

    if(letterCountArray.first.length != letterCountArray.second.length) {
        return "Not an anagram!"
    }

    for(let i = 0; i < letterCountArray.first.length; i++) {
        let firstPair = letterCountArray.first[i]
        let secondPair = letterCountArray.second[i]

        if(letterCountArray.first.filter(pair => pair.letter == secondPair.letter && pair.count == secondPair.count).length != 1) {
            return "Not an anagram!"
        }

        if(letterCountArray.second.filter(pair => pair.letter == firstPair.letter && pair.count == firstPair.count).length != 1) {
            return "Not an anagram!"
        }
    }

    return "Anagram!"
}

I'm also curious about a better solution :)


Abstract Data Type: Stack implementation in C++

It seems a stack is the HelloWorld of C++ classes and memory management :).

First, your class has no destructors. It means that when your instance will be destroyed, it will only deallocate the stack attributes : top, level and max_size. Notice that it deallocate the pointer top, but not the object pointed by top ! You must define a destructor : ~Stack(), which will likely consists in a loop executing pop() while the stack is not empty.

Remember that when you are encapsulating pointers, you must respect the Rule of Three (known also as Rule of Five) : it is a simple but accurate rule which can be interpreted as :

  If you need to specify one of destructor/Copy Constructor/Copy Assignment Op; then you probably have to define all three.

Here, it applies because you are holding a pointer, whose value is deleted in the destructor. So if you do the default copy, for instance Stack<T> s2 = s1;, it will copy the data members of s1 into s2. It means that s2.top == s1.top. So when one get destroyed, for instance s1, it will delete s1.top, but then s2.top points to deleted memory : ERROR.

While you are defining copy-constructor and copy-assignment, you may want to define move-constructor Stack(Stack&& other) and move-assignment Stack& operator=(Stack&& other). T&& means "Movable T" and signifies "This value is temporary, so there is no problem if it is altered". For example, here, it means you can only swap other.top and this->top, so you do not need to copy each node of the stack.
Both move-constructor and move-assignment are parts of the Rule of Five, which is an extent of the Rule of Three above.

You use std::cerr to showing errors. It is not the right way to do this : for example, if push() results in an error, the code calling push has no way to know it. The program will continue to run as usual.

C++ has a specific way to handle errors : exceptions. I will not detail here but follow the link and give it a try.

Moreover, in your pop() function, there is a problem. If the stack is empty, there is no return value ! Your compiler should at least warn you (if not yelling at you) that one conditional-branch has no return value. Instead, you should throw an exception, or, if it is intended, use std::optional<T>. std::optional can represent two states : empty, or full. If it is full, you can access to T value.

Do you know the keyword const in method declaration ? It means it will not modify the object during its execution. Examples :

bool method_A() const; //declaration
int method_B() const { /* definition */ }

You need the keyword const in both declaration and definition. Currently, the functions which can be marked const are display(), isEmpty(), isFull(), getLevel() and getMaxSize().

One of the purpose is the const-ref argument. For example :

template<typename T>
void printLevel(Stack<T> const& stack) {
    std::cout << "Nb levels = " << stack.getLevel() << std::endl;
}

The argument type is a const-ref (one & means reference). It means that, because it is a reference, the argument is not copied, but you can not modify it : "read-only" (because of const). Having a const-ref variable means you can only access to const methods.

Also, you can optimize memory management by using smart pointers. I will not explain this here, but please check out my answer to another stack implementation, which details how can smart pointers be used.

Something you should do is defining max_size constant :

public:
    const int max_size;

Notice that this is ok to set max_size public, because it is a constant and it won't be modified by external code.


Generating variables to be named, filled in and operated on the fly in Python

Just a little something to clean up the code.

The lines inside your for loop e.g

open_cost_mat_train = np.append(open_cost_mat_train,i)

do not require an assignment on the left, np.append modifies the array being passed in. I'm not sure if that's what you want, but maybe you should inspect the arrays after the loop to ensure they contain what you expect.

If you're worried about modifying arrays by mistake or require a original copy, it may be worth cloning them before running any operations on them.

copy_array = original_array.copy() will clone a numpy array.

Another way to verify you're passing the right parameters could be to check their names. They seem to be labelled logically with a number at the end, so it seems closed_cost_mat_train_2 should always go with closed_cost_category_path_2. See here, the inspect module looks like a good choice for this.

Docs for the numpy functions:

[1] numpy.append

[2] numpy.copy


Git commit logger for daily stand-up meeting

A much much much much better way of doing this is:
find . -type f -exec git --no-pager log --pretty=format:'%Cred%h%Creset -%Creset %s %Cgreen(%cD) %C(bold blue)<%an>%Creset' --since yesterday {} --author $gitUsername \;
And a cleaner way to write long statements it to add '\' to break into new lines.
find . -type f \
-exec git --no-pager log --pretty=format: \
'%Cred%h%Creset -%Creset %s %Cgreen(%cD) %C(bold blue)<%an>%Creset' \
--since yesterday {} \
--author "${gitUsername}" \;

This way, 7 lines are written as 1 and to stdout.

--no-pager returns everything to your terminal stdout instead your editor.
find . says which directory to search in, so a for loop may be useful to only search desired directories
Also, explicitly defining the desired dirs will remove the need to redirect to stderr and when useful errors are displayed, will be meaningful to you.

https://linux.die.net/man/1/find
https://www.golinuxcloud.com/find-exec-multiple-commands-examples-unix/


With the premise that you're not allowed to use Arrays.sort It'd be more succinct to write your own sorting method.

All you really need to do is initialize an array of the size, count the 1s and set that many positions from the end to 1.

This takes advantage of the fact that integers in java are initialized to 0; the resulting solution becomes straightforward.

Try it yourself, then look at this implementation:

public class Segregate {
  public static void main(String[] args) {
    int[] a = {1, 0, 1, 0, 1, 1, 0, 1, 1, 1};
    System.out.println("Before segregation: " + join(a, ", "));
    System.out.println("After  segregation: " + join(segregate(a), ", "));
  }

  private static int[] segregate(int[] onesAndZeros) {
    int[] segregated = new int[onesAndZeros.length];
    int end = segregated.length - 1;
    for (int count = countOnes(onesAndZeros); count > 0; count--) {
      segregated[end--] = 1;
    }
    return segregated;
  }

  // Obviously only works in this specific case.
  private static int countOnes(int[] onesAndZeros) {
    int count = 0;
    for (int n : onesAndZeros) {
      count += n;
    }
    return count;
  }

  private static String join(int[] nums, String delim) {
    StringBuilder joined = new StringBuilder();
    for (int num : nums) {
      joined.append(delim).append(num);
    }
    return joined.substring(delim.length());
  }
}

Output:


C# Update account based on user order data. Nested if-else optimization with almost similar values

This code is hard to read, deeply nested and hard to understand. But you already know that.
Since you have a lot of binary cases (if-else) I suggest creating a truth table
on paper or whiteboard https://en.wikipedia.org/wiki/Truth_table#Binary_operations for your different combinations of input and output.
This should help you figure out which cases can be simplified, short-circuit, etc, and then you can rewrite the code to make it more readable. Use early returns and maybe refactor it into functions that allow you to check error conditions and return/exit as soon as possible for each case.
The end goal should be a code structure that doesn't nest 4 5 levels deep, preferrably no more than 2.


Riemann Sum Class (with variable endpoint behavior)

If something is called "RiemannSum", then it should return a sum. If you want summation to be a separate method, a better term would be "partition"; you could do RiemannPartition.sum(), for instance.

Several other variable names are bit unintuitive to me (e.g. rectangles, index).

You can eliminate several lines by doing:

offset = {'left':0,'right':self.delta,'mid':self.delta/2}[self.endpoints.lower()]
self.points = arange(self.index+offset, self.upper+offset, self.delta)

General tip: if you're doing an if-then with several elif/else clauses, consider whether a dictionary would be simpler.


Creating a repetitive string in Common Lisp

Not a bad approach.

I can see one arguable bug, and have a few style notes on your code though. Lets start with the bug:

CL-USER> (defparameter a "test")
A
CL-USER> (pad-with-string 6 a :from-end t)
"tsetts"
CL-USER> a
"tset"
CL-USER>

Is that what you were expecting? It's happening because you call nreverse on your input argument if from-end is passed. When I've got a situation where I have to mutate parameters, I follow the Scheme convention and name the procedure with a trailing bang (so, like pad-with-string!). Your comments state that you don't care about efficiency though, which leads me to believe you could just use the standard reverse rather than nreverse.

You're using apply on circular list, but you're defining it locally and only calling it in one place. In this situation, I'd kill the &rest and just call the function

...
  (labels ((circular-list (elements)
         (let ((cycle (copy-list elements)))
           (nconc cycle cycle))))
    (loop :repeat n
       :for c
       :in (circular-list (coerce (if from-end (nreverse str) str) 'list))
...

You've got the (if from-end (reverse a) a) pattern in a couple of places. I'd pull that out into an additional local definition.

...
       (maybe-reverse (thing)
         (if from-end (reverse thing) thing)))
    (loop :repeat n
       :for c
       :in (circular-list (coerce (maybe-reverse str) 'list))
       :collect c
       :into chars
       :finally (return (coerce (maybe-reverse chars) 'string)))))

thanks to Rainer for pointing this out

The result of the collect call is returned implicitly at the end of the loop unless you give it an explicit name. Which means you can avoid finally (return ...) by wrapping those transformations around the loop itself:

...
(coerce
 (maybe-reverse
  (loop :repeat n
     :for c
     :in (circular-list (coerce (maybe-reverse str) 'list))
     :collect c))
 'string)))

Not a hard and fast rule, but people typically use symbols rather than keywords for loop words. So,

...
(coerce
 (maybe-reverse
  (loop repeat n
     for c
     in (circular-list (coerce (maybe-reverse str) 'list))
     collect c))
 'string)))

Again, there's not really a standard for this, but I tend to like putting conditions and their effects/modifiers on the same line.

...
(coerce
 (maybe-reverse
  (loop repeat n
     for c in (circular-list (coerce (maybe-reverse str) 'list))
     collect c))
 'string)))

So,

(defun pad-with-string (n str &key (from-end nil))
  (labels ((circular-list (elements)
             (let ((cycle (copy-list elements)))
               (nconc cycle cycle)))
           (maybe-reverse (thing)
             (if from-end (reverse thing) thing)))
    (coerce (maybe-reverse
              (loop repeat n
                 for c in (circular-list (coerce (maybe-reverse str) 'list))
                 collect c))
            'string)))


SICP Exercise 1.3: Sum of squares of two largest numbers out of three, Rust Version

I find it always pays to be tidy and conscientious about spacing. Here, I'd add spaces after the : in the argument declaration

// fn sum_square_largest(x:f64, y:f64, z:f64) -> f64 {
fn sum_square_largest(x: f64, y: f64, z: f64) -> f64 {

Next, I'd wrap the where clause onto the next line. where clauses often get large, so I find it best to let them have breathing room, and only put one type per line:

// fn partial_min_three<T>(v1: T, v2: T, v3: T) -> Option<T> where T: PartialOrd<T> {
fn partial_min_three<T>(v1: T, v2: T, v3: T) -> Option<T>
    where T: PartialOrd<T>
{

Checking for Some or None and doing something is a common pattern. In this case, you can use Option::and_then:

// match partial_min(v2, v3) {
//     Some(x) => partial_min(v1, x),
//     None => None
// }
partial_min(v2, v3).and_then(|x| partial_min(v1, x))

Programming is often about clarity in expressing your intentions, and I doubt that the majority of programmers immediately know what 0 / 0 is for floating point numbers. You should be explicit:

// None => 0.0_f64/0.0_f64
None => std::f64::NAN,

Again, we can use existing patterns to deal with checking for Some / None. This time, we can use Option::map:

// match partial_min_three(x, y, z) {
//     Some(a) => x * x + y * y + z * z - a * a,
//     None => std::f64::NAN,
// }
partial_min_three(x, y, z)
    .map(|a| x * x + y * y + z * z - a * a)
    .unwrap_or(std::f64::NAN)

I disagree with your choice of NaN as a magic value. Magic values drive me crazy, and Rust has great choices for avoiding them - Option and Result!

fn sum_square_largest(x: f64, y: f64, z: f64) -> Option<f64> {
    partial_min_three(x, y, z)
        .map(|a| x * x + y * y + z * z - a * a)
}

I'd probably create a tiny function for squaring. It's likely to get inlined, so I'm not worried about performance, just readability:

.map(|a| square(x) + square(y) + square(z) - square(a))

All together (playpen):

fn sum_square_largest(x: f64, y: f64, z: f64) -> Option<f64> {
    partial_min_three(x, y, z)
        .map(|a| square(x) + square(y) + square(z) - square(a))
}

fn partial_min_three<T>(v1: T, v2: T, v3: T) -> Option<T>
    where T: PartialOrd<T>
{
    partial_min(v2, v3).and_then(|x| partial_min(v1, x))
}

fn square(x: f64) -> f64 { x * x }

// Copied from the standard library as it is currently unstable
// and the beta doesn't allow unstable features
use std::cmp::Ordering;
fn partial_min<T: PartialOrd>(v1: T, v2: T) -> Option<T> {
    match v1.partial_cmp(&v2) {
        Some(Ordering::Less) | Some(Ordering::Equal) => Some(v1),
        Some(Ordering::Greater) => Some(v2),
        None => None
    }
}

fn main() {
    let z = sum_square_largest(1.0, 2.0, 3.0);
    println!("{:?}", z);
}


Checking consecutive characters in a string

By using the Distinct method and comparing lengths you can get the number you want in one line.  Here's what such a function could look like:

Function CharsToDelete(Of T)(input As IEnumerable(Of T)) As Integer
    Return input.Count - input.Distinct.Count
End Function

Oops missed the part about consecutive chars.  This should do the trick:

Function CharsToDelete(Of T)(input As IEnumerable(Of T)) As Integer
    Return input.Where(Function(x, y) x.Equals(input(y + 1))).Count
End Function

You don't need to split the text into an array.  The Lines property of the TextBox already does this:

Dim inputArr As String() = txtInput.Lines


tmp is never a good name for a variable.  In this case, you should call it token instead, or perhaps word.  And you rightly complain that building strings using repeated += operations is inefficient, and correctly suggest that .substring() would be better.

Logically, then, you need to find the starting and ending indexes of each token!  So, let's define those helper functions (as private static functions):

/**
 * Considering str starting at startIndex, find the index at which the
 * next token starts.
 *
 * @return The index of the start of a token (or str.length() if no more
 *         tokens).
 */
private static int start(String str, char sep, int startIndex) {
    int i;
    for (i = startIndex; i < str.length() && str.charAt(i) == sep; i++);
    return i;
}

/**
 * Considering str starting at startIndex, find the index at which the
 * current token ends.
 *
 * @return The index just beyond the end of a token (the index of a
 *         sep character, or str.length() if this is the last token)
 */
private static int end(String str, char sep, int startIndex) {
    assert(str.charAt(sep) != sep);
    int i;
    for (i = startIndex; i < str.length() && str.charAt(i) != sep; i++);
    return i;
}

Then, we can use them in tokenize():

public static String[] tokenize(String str, char sep) {
    int tokenCount = 0;
    for (int s, e = 0; (s = start(str, sep, e)) < str.length(); e = end(str, sep, s)) {
        tokenCount++;
    }
    String[] tokens = new String[tokenCount];
    tokenCount = 0;
    for (int s, e = 0; (s = start(str, sep, e)) < str.length(); ) {
        tokens[tokenCount++] = str.substring(s, e = end(str, sep, s));
    }
    assert(tokens.length == tokenCount);
    return tokens;
}

Notice that now you can take advantage of the helper functions to predetermine the size of the array.  Also, a lot of the repetitiveness of your conditions and loops is eliminated.

All of the analysis is done using string indexes, so there is no string manipulation other than just the essential .substring() calls — and even the trim() call is gone!


Try moving the ofstream outside of the method so that you're not creating it every time you make a recursive call.

Like this:

#include <stdio.h>
#include <fstream>

using namespace std;

int hanojus(int n, char a, char b, char c, ofstream& fr)
{
    if (n == 1)
        fr<<a<< " -> " <<c<<endl;
    else
    {
        hanojus(n-1, a, c, b, fr);
        fr<<a<< " -> " <<c<<endl;
        hanojus(n-1, b, a, c, fr);
    }
}

int main()
{
    ifstream fd("hanoj.in");
    ofstream fr("hanoj.out",ios::app);
    int sk;
    fd>>sk;
    hanojus(sk, 'A', 'B', 'C', fr);
}


C String - new function to detect user's anger

Algorithm:

There are a few fallacies that you assume in your approach.  Nothing wrong, but things that could be improved:

You assume bounds on your "heat score" with this comment

/* 0% cold; 100% very heated */

But there is no code implementing these bounds.  You can go negative, as well as go above 100.  I'd recommend observing these bounds, and sentiment should be seen as a probability which can only be between 0 and 1.

To match this probability, it might be better for you to store your sentiment score as a double rather than an int but that choice is up to you.
Right now you are using a bag-of-words model.  This is a typical approach when first starting out with sentiment analysis since it is easier, but it usually gives a lower accuracy representing what the actual sentiment of the text is.

As I was saying, it's a fairly straightforward and practical way to go, but there are a lot of situations where it will get make mistakes.

Ambiguous sentiment words - "This product works terribly" vs. "This product is terribly good"
Missed negations - "I would never in a millions years say that this product is worth buying"
Quoted/Indirect text - "My dad says this product is terrible, but I disagree"
Comparisons - "This product is about as useful as a hole in the head"
Anything subtle - "This product is ugly, slow and uninspiring, but it's the only thing on the market that does the job"

As far as NLP helping you with any of this, word sense disambiguation (or even just part-of-speech tagging) may help with (1), syntactic parsing might help with the long range dependencies in (2), some kind of chunking might help with (3). It's all research level work though, there's nothing that I know of that you can directly use. Issues (4) and (5) are a lot harder, I throw up my hands and give up at this point.
You are going to not score a decent amount of sentences that occur in actual life.  Take a look at a lot of sentences in this post, for example.  They don't contain swear words, "please" or "sorry", exclamation points or words in caps.  You need a more general lexicon of positive, negative, and neutral words and then a system to weigh in the effects of these words into your score.
There are some odd score modifications you do.  Why is it when I refer to myself with "I", that the score is considered more positive?  I don't think it should, and I would say to reconsider the reason you think so.

Sentences that end with an exclamation point aren't necessary negative (higher heat) either.  An exclamation point is often used to indicate strong feelings (such as excitement) or high volume.  Most sentiment analysis systems that I've looked at don't consider punctuation in the final score at all.

"A" should be considered the same as "I".  It is very possible that a sentence could start with "A", and be either positive or neutral but your program considers it to have a negative connotation.

For a basic sentiment analysis this is fine, but do note that it does have it's flaws.  If you're looking to improve the accuracy of your algorithm, I'd recommend reading this research paper, which achieves a classification accuracy of 90% (higher than any other published results).

Code:

Right now you have the method str_findString().  I'm guessing this is an variation of strstr(). I'm also guessing that <string.h>'s implementation of this method will be more efficient and faster, based on it being a standard library.

if (strstr(temp_a, "$#@#"))
{
    ...
}

Declare i inside of your for loops.(C99)

for (int i = 0; i < objString->length; i++)

I would add another tab to the function body.

EHeat str_isHeated(STRING *objString)
{
    int intHeatScore = 0;       /* 0% cold; 100% very heated */
    STRINGCOLLECTION tokens;

I would combine your last two return conditions into one.

else if (intHeatScore >= 0)
    return EHeat_Cold;

return EHeat_Cold;

I find the last else-if comparison useless overall, and would just return EHeat_Cold anyways if it wasn't included.

return EHeat_Cold;

I'm not too sure about what I'm assuming are #defines: STRING and STRINGCOLLECTION.  I guess it is okay to keep them, but is there a specific reason you don't just put in what they actually are: char* and char* array respectively?


if(st.isEmpty())
  return true;
 else return false;

This can be simplified to return st.isEmpty();... Wait, that's not the right advice.

Why do you even need a Stack when you can simply count?

public static boolean check(String s) {
    int counter = 0;
    for (int i = 0; i < s.length(); i++) {
        if (s.charAt(i) == '(') {
            counter++;
        } else if (s.charAt(i) == ')') {
            if (counter == 0) {
                return false;
            }
            counter--;
        }
    }
    return counter == 0;
}

Other issues (which may or may not be picked up by the automatic assessment):

If this is assessed using Java 7, using try-with-resources is recommended for the BufferedReader instance.
You can inline the printing of the results, i.e. System.out.println(check(input));.
Please use braces consistently... that tends to improve code readability for human programmers.

edit: OK, second stab at a Java 8 stream-based solution...

// Using better-suited visibility modifier and method name
private static boolean balanced(String input) {
    return input.chars().mapToDouble(i -> i == '(' ? 1 : i == ')' ? -1 : 0).reduce(0,
            (a, b) -> a == 0 && b == -1 ? Double.NaN : a + b) == 0;
}

mapToDouble() is used here just so that Double.NaN can be used as a special placeholder to effectively ignore further computations when a ) is encountered without a matching (. It doesn't look as streamlined as I hoped it would be though, and I think for most intent and purposes the non-stream-based way works and reads just a little better.


Binary search on string in alphabetical order

Binary search is usually simpler:

function binarySearch(haystack, needle) {
    var a = 0;
    var b = haystack.length - 1;
    if (needle < haystack[0] || needle > haystack[b]) {
        return false;
    }
    while (a < b - 1) {
        var c = (a + b) / 2 |0;
        if (needle < haystack[c]) {
            b = c;
        } else {
            a = c;
        }
    }
    return haystack[a] === needle || haystack[a+1] === needle;
}


Typescript & rxJS group list of object by month/year and map to certain value

You're using RxJS, but your inputs and outputs are plain arrays. It'd be simpler to either consume and return streams or replace RxJS with Lodash/Ramda/Underscore/native array methods/some iteration-based library.
(If you want to get really fancy or need to use the function in different contexts, you could write a transducer.)
You're using a class, a method and a static property, but it would be sufficient to have a type, a function and a module-local constant. And passing datePipe (or just a Date) and status => getPointsFromStatusToPointsMap(status) function as arguments would make the dependencies more explicit. But these are more like code style preferences.
Lastly, you're returning any, but you should always return unknown instead unless you really have to lie to the type system for some reason. And returning [string, number] or [Date, number] would be more convenient for the consumer.


Implement code coverage flush function with boiler plate code

This works (at least I assume it does), but it's not how I'd do things.

I'd put the code:

void cscomm_gcov_flush()
{
    __gcov_flush();
}

...into a file by itself (e.g., flusher.c or flusher.cc, as you prefer). Then I'd have the build tool decide whether to compile and link that into the .so files depending on whether it's building an instrumented binary or not.

OBJS = // file list here

ifdef __CODE_COVERAGE__
    OBJS += flusher.o
endif

Syntax varies with the build tool you're using of course, but I think the general idea is fairly apparent.

Looking at the code more generally, names that start with an underscore followed by another underscore or a capital letter (including __CODE_COVERAGE__) are all reserved for the implementation, so defining them leads to undefined behavior. If the compiler does it in response to a command line argument (or similar) that's fine, but if you're doing it yourself, you have undefined behavior.


The first thing that springs to mind is that it would probably be better to not create the Stream and StreamReader instances when it can be avoided simply by using the File.ReadAllText method to accomplish the same goal:

public void ProcessFile(string filename)
{
   Console.WriteLine(File.ReadAllText(filename));
}

In other situations you might need to avoid this, especially for very large files.  For small files and simple operations the File.Read... methods should suffice.

Main advantage is that you relegate the object management issues to the framework, reducing the complexity of your code.  This should result in fewer bugs and greater readability.

For larger files, especially where you are wanting to process individual lines (a common use case) it is often useful to use the File.ReadLines method to process the content.  This should read the lines as needed with some buffering, reducing memory consumption while maintaining code simplicity.

On the question of exception handling... I prefer to only put exception handling in when it is required by the specific situation, and I didn't see a specific need here.  If the idea of an exception breaking out of this code concerns you then by all means add some in.  Personally I'd prefer to have the exception bubble out than have to write all the extra code in here to not only handle the exception but also change what we do with a null return, etc.

In general I don't like the idea of always trying to catch exceptions as soon as possible.  I prefer to catch as soon as I need to and no sooner.


As indicated by the scattered comments on https://stackoverflow.com/q/3349753/2943403, your approach is trustworthy.

scandir() has an advantage over glob() (which is normally handy when trying to ignore . and ..) because glob() will not detect hidden files.

The RecursiveIterator methods are powerful, but it is my opinion that fewer developers possess the ability to instantaneously comprehend all of the calls and flags (and I believe that should weigh in on your decision).

As for your snippet, I would like to clean it up a little.

public static function delTree($dir) {
    if (!is_dir($dir)) {
        return false;
    }

    $files = scandir($dir);
    if (!$files) {
        return false;
    }
    $files = array_diff($files, ['.', '..']);

    foreach ($files as $file) {
        if (is_dir("$dir/$file")) {
            SELF::delTree("$dir/$file");
        } else {
            unlink("$dir/$file");
        }
    }

    return rmdir($dir);
}

I don't use ternary operators when I am not assigning something in that line.  For this reason, a classic if-else is cleaner in my opinion.


Game server auth: cookie key generate&check

We have better random generators in C++ than std::time(0); if you're using the latter for its speed, it would really be worth a comment in the code to explain that you're willing to live with such a weak random-number source in exchange for the speed.

Note that embedding the secret in the code will make it visible to anyone who can obtain the source or the binary and read it.  I'd recommend that you read it at runtime from a separate file - it's easier to correctly permission that one file rather than your source and all the build products.

For the case where the id is specified, there's no need to compute a time-based one just to throw it away immediately - I'd turn that conditional around:

std::string gameKey(long long id = -1)
{
  if (id == -1) {
    id = std::time(0) * 1000 + dateCounter++;
  }

Alternatively, write an overloaded function pair:

std::string gameKey()
{
   return gameKey(std::time(0) * 1000 + dateCounter++);
}

std::string gameKey(long long id)

A couple of misspellings: std::size_t and std::uint8_t (both missing namespace prefix in the code - it might work on your platform, but it's not portable).

I think that key can be a const char* or even a string literal:

blake2b(out, in, "", outlen, inString.length(), 0);

General: please give your code a bit more room to breathe - bunching up the source with no whitespace won't make it any more efficient.

Given that outlen is constant, we could declare it constexpr - then we wouldn't need to allocate out, but could declare it in automatic storage.  There's certainly no need to allocate writer and have to delete it - just make it an automatic variable:

Json::Writer writer;
return writer.write(obj);
// look, no delete needed!

If you do need a variable outlen, at least use a smart pointer, so it's cleaned up for you.

Instead of writing to 4 buffers a, b, c and d, it might be simpler to write to a single buffer, and copy substrings from there (it would reduce those repeated loops with similar-but-different ranges).


Reduce duplication while accessing Visual Studio services

Your current design will potentially make unit testing code which uses it hard. Consider making the VisualStudioServices class non-static and create an interface for it. You could then inject the ServiceProvider as a dependency rather having an implicit dependency on the static GlobalProvider. Something like this:

interface IVisualStudioServices
{
    IVsUIShell UIShell { get; }
}

class VisualStudioServices : IVisualStudioServices
{
    public IVsUIShell UIShell { get { return _VsUiShell.Service; } }

    private readonly LazyService<SVsUIShell, IVsUIShell> _VsUiShell;

    public VisualStudioServices(ServiceProvider serviceProvider)
    {
        _VsUiShell = new LazyService<SVsUIShell, IVsUIShell>(serviceProvider);
    }

    class LazyService<TService, T>
    {
        ServiceProvider _ServiceProvider;
        LazyService(ServiceProvider serviceProvider)
        {
            _ServiceProvider = serviceProvider;
        }

        readonly Lazy<T> serviceLazy = new Lazy<T>(() => (T)_ServiceProvider.GetService(typeof(TService)));

        public T Service { get { return serviceLazy.Value; } }
    }
}

Any code requiring access to it should have a dependency on IVisualStudioServices which can be easily mocked out.


I like your solution, it's clever & fast, however I can't help but feel like it goes against the spirit of the assignment.  You've found a built in system call that greatly simplifies what it is your need to do, which is great, but in doing so you've delegated the algorithm, which may not be what is expected from the assignment.

The assignment reads as if it's expecting your to use things like opendir and readdir to iterate through the folders yourself.  You would then be responsible for maintaining state, and writing the iteration algorithm.

-name or not -name

Your argument handling is also a little bit on the naive side.  Should I really be able to run it like this:

./my_find ../../ -ThisArgumentIsNeverCheckedSoICanPutPrettyMuchAnything "CMakeLists.txt"

Why both having the second argument if you're not going to check it, it's just extra typing...

Wildcards

It's not really clear from the spec, however the built in 'find' command supports wildcards.  Your current implementation doesn't appear to (you're only doing an exact match).


Finding the last ten digits of \$\sum_{n=1}^{1000} n^n\$

Use the right data type. The last ten digits of the sum is same as the sum modulo \$10000000000 = 10^{10}\$, which is smaller than \$2^{34}\$. All computations can be comfortably done with 64-bit integers; invoking BigInteger is a definite overkill. Extracting ten digits via string operations is also quite suboptimal.
Reuse your computations. Once you computed \$k = n^n\$, use it to compute \$(2n)^{2n} = 2^{2n} n^{2n} = 2^{2n}k^2\$.
Devise an algorithm. The logic of the previous bullet applies to any \$mn\$: \$(mn)^{mn} = m^{mn}n^{mn} = (m^m)^n (n^n)^m\$. This observation lends itself to the scheduling of computations very close to be optimal. Think about divisibility in general and prime numbers in particular.
Any multiple of 10 raised to the corresponding power is surely divisible by \$10^{10}\$, and can be safely omitted from summation, but this is a minor optimization.


More efficient solution for Project Euler #2 (sum of Fibonacci numbers under 4 million)

A hint that I always offer people with this specific question. Take a look at the first few numbers of the sequence - I have set the even ones in bold:

1, 1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144...

Can you see a pattern in the spacing? Can you explain (ideally, prove) it?

Given that, can you think of a way to generate even Fibonacci numbers directly, instead of filtering through for them? (Hint: start with (1,2), and try to generate (5,8) in one step, then (21, 34) etc. Do some algebra and figure out how to apply multiple iterations of the generation rule at once.)

Anyway, in terms of doing the actual calculation with the numbers, there are more elegant approaches. Instead of asking Python to build a list directly, we can write a generator:

def even_fibonacci_up_to(n):
    a, b = 1, 2
    while b <= n:
        yield b # since 2 qualifies, after all.
        # Now calculate the next value.
        # math from the first step goes here when you figure it out!

Notice the use of the yield keyword. This lets us treat our expression even_fibonacci_up_to(4000000) as a special sort of sequence whose elements are generated on-demand. Unlike with ordinary functions which can only return once. :) (Using return inside a generator terminates the whole process of yielding values when it is reached.)

And instead of looping through this sequence to add up the numbers, we can use the built-in sum function:

sum(even_fibonacci_up_to(4000000))


I assume you have a typo there, error is nowhere defined but you do have err.

To jackdbernier's point, you should read the docs:
http://expressjs.com/guide.html#users-online

In there it is clear that you should call next regardless whether you have an error or not, otherwise not all middlewares will be processed.

If you insist on logging before call next, you could.

function handleError( message , err , next )
{
  console.error( message );
  return next( err );
}

new Category({
  name: name,
  description: description,
}).save(function(err, docs){
  if ( err ){
    return handleError( 'error: saving category' , err, next );
  }
  console.log('category saved.');
  res.redirect('categories');
  next(); // <- this!
});
}

Though I would personally drop the curly braces from that if( err ).


Here is my first review. See what you can make of it! I left comments for you. They always discuss the code right below.

It is bad style to leave documentation tags empty. Either have no param tag or fill all with appropriate descriptions:

/// <param name="content"></param>
/// <param name="SearchString">Search string</param>
/// <param name="NumberOfMatches">Specify how many words have to match in the search string.

Too much implementation detail in my opinion:

/// This value is automatically dropped when the user searches less words.
/// I.e. NumberOfMatches = 3 and the user searches 'Bob', NumberOfMatches gets set to 1.</param>

Self explanatory name:

/// <param name="MatchAllWords">If set to true, all of the words in the search string must be contained in what's being checked</param>

This is self explanatory, remove the line:

/// <returns></returns>

Use a more telling name like ContainsWords:

public static bool ContainsV2(this string content, string SearchString, int NumberOfMatches = 1, bool MatchAllWords = false)
{
    var matches = 0;
    var numberOfWords = SearchString.Split(' ').Length;

Don't use empty comments like "Sets a variable" or "Updates value". Explain it better or leave out the comment:

    // Update required number of matches

The ?: operator is hard to understand and not necessary here. Rather, use something like in my comment:

    // if (NumberOfMatches > numberOfWords)
    // {
    //    NumberOfMatches = numberOfWords;
    // }
    NumberOfMatches = NumberOfMatches > numberOfWords ?
                    numberOfWords :
                    NumberOfMatches;

You already split the search string above. Either don't check for null or do it in the beginning:

    if (!string.IsNullOrEmpty(content) && !string.IsNullOrEmpty(SearchString))
    {

This is confusing. Reduce the complexity and nesting depth:

        SearchString.Split(' ')
            .ToList()
            .ForEach(w => //Gets the index of each word after being split
                matches += content
                    .IndexOf(
                        StripNonAlphaNumeric(w),
                        StringComparison.OrdinalIgnoreCase
                    ) >= 0 ? 1 : 0
            );
    }

Rather set this at the start of the function. It is confusing here:

    if (MatchAllWords)
    {
        return matches == numberOfWords;
    }
    return matches >= NumberOfMatches;
}


Calculating the minimum and maximum of a byte array

Your combined function essentially looks like this:

// commonly used variables
if (min) {
    // code for computing the min
    return minValue;
} else if (max) {
    // code for computing the max
    return maxValue;
} else {
    return BAD;
}

Since there is no common code, you should either:

put the calculation for min and max in two separate functions
merge the code (which makes the code a little slower)

In the current code the variable definitions are too far away from the code that actually uses them. Understanding the code is easier when everything fits on a single screen.

Continuing with your code:

#define GOOD 1
#define BAD 0

These two constants may come from the 1980s or 1990s, when <stdbool.h> was not available yet. Nowadays functions that return success or failure should do so by returning bool instead of int.

typedef unsigned int uint;
typedef unsigned char byte;

typedef enum uMaxOrMin
{
    Max,
    Min
} MaxOrMin;

Since you don't use the uMaxOrMin name later, you can omit it.

typedef enum uBitStyle
{
    BitsOn,
    BitsOff
} BitStyle;

This whole enum is unused and should therefore be removed.

int MaxOrMinValue(byte src[], uint size, byte *val, MaxOrMin mom)

This function is not supposed to modify the contents of the src array. Therefore it should be declared as const byte src[].

To avoid the common confusion between * and [] for parameters (only the pointer is passed, not the whole array), it is preferable to write const byte *src instead.

The name of the size param is not as precise as possible. It should rather be src_size to make it unambiguous that it belongs to src.

{
    uint srcTemp;

The only time where temp is allowed as part of a variable name is in the popular pattern to swap two variables: tmp = a; a = b; b = tmp. In all other situations there must be a better name.

    uint srcIndex = 0;

Since there is only one thing that can be indexed here, the src in srcIndex is redundant. The code is short enough that the variable names can be short, too.

    uint largest;
    uint smallest;
    uint* data = src;

    switch (mom)
    {
    case Max:
    {
        largest = src[0]; //initialize largest

Accessing src[0] is only allowed if src_size > 0.

        while (*data++) //iterate over binary array
        {
            srcTemp = *(src + srcIndex++); //start at beginning of array, grab first byte and increment by 1

There is no point in writing *(ptr + index) since ptr[index] is much clearar to read.

            if (srcTemp > largest) //current byte is greater than our saved largest byte

Ah, so srcTemp really means "the current byte". In that case, the variable should be named curr.

                largest = srcTemp; //set new largest
        }

        *val = largest; //return max to caller

In this assignment, you are storing an unsigned int into a byte variable. Because both are unsigned, this works. Better style would be to make largest a byte variable, too. In other programming languages like Go or Kotlin, assigning values between different-sized variables is a compile-time error, and I find these quite helpful.

        return GOOD;
    }

    case Min:
    {
        smallest = src[0]; //initialize smallest
        while (srcIndex < size) //iterate over binary array

Since min and max are essentially the same algorithms, their code should express this by being essentially the same. You are using two entirely different loops here, which leads to confusion and bugs.

        {
            srcTemp = *(src + srcIndex++); //start at beginning of array, grab first byte and increment by 1
            if (srcTemp < smallest) //current byte is less than our saved smallest byte
                smallest = srcTemp; //set new smallest
        }

        *val = smallest; //return min to caller
        return GOOD;
    }
    return BAD; //if we hit here, we've failed
    }
}

All in all, your code does not take any advantage of the algorithms being so similar. To combine them in a single piece of code, I suggest this:

#include <assert.h>

#define GOOD 1
#define BAD 0

typedef unsigned int uint;
typedef unsigned char byte;

typedef enum {
    Max,
    Min
} MaxOrMin;

int MaxOrMinValue(const byte *src, uint src_size, byte *result, MaxOrMin mom) {
    if (src_size == 0) {
        return BAD;
    }
    if (mom != Min && mom != Max) {
        return BAD;
    }

    byte mask = (byte) (mom == Max ? -1 : 0);
    byte best = src[0] ^ mask;
    for (uint i = 1; i < src_size; i++) {
        byte curr = src[i] ^ mask;
        if (curr < best) {
            best = curr;
            if (best == 0)
                break;
        }
    }

    *result = best ^ mask;
    return GOOD;
}

int main() {
    byte arr1[] = {0, 1, 2, 3, 4};
    byte arr2[] = {200, 100, 222};
    byte minmax = 13;

    assert(MaxOrMinValue(arr1, sizeof arr1, &minmax, Min) == GOOD);
    assert(minmax == 0);

    assert(MaxOrMinValue(arr1, sizeof arr1, &minmax, Max) == GOOD);
    assert(minmax == 4);

    assert(MaxOrMinValue(arr2, 0, &minmax, Max) == BAD);
    assert(minmax == 4); // unchanged

    assert(MaxOrMinValue(arr2, 1, &minmax, Max) == GOOD);
    assert(minmax == 200);

    minmax = 123; // to make sure it is overwritten again
    assert(MaxOrMinValue(arr2, 2, &minmax, Max) == GOOD);
    assert(minmax == 200);

    assert(MaxOrMinValue(arr2, 3, &minmax, Max) == GOOD);
    assert(minmax == 222);

    // Passing neither Min nor Max is bad.
    assert(MaxOrMinValue(arr2, 3, &minmax, 123) == BAD);
    assert(minmax == 222); // unchanged
}

Admitted, using a bitmask to combine the min and max algorithms is tricky. Without using this trick, the code might look like this:

int MaxOrMinValue(const byte *src, uint src_size, byte *result, MaxOrMin mom) {
    if (src_size == 0) {
        return BAD;
    }
    if (mom != Min && mom != Max) {
        return BAD;
    }

    byte best = src[0];
    byte best_possible = mom == Max ? (byte) -1 : (byte) 0;
    for (uint i = 1; i < src_size; i++) {
        byte curr = src[i];
        if (mom == Max ? curr > best : curr < best) {
            best = curr;
            if (best == best_possible) {
                break;
            }
        }
    }

    *result = best;
    return GOOD;
}

Oh, nice. The code even got simpler. One thing that bothered me though about this code is the complicated condition in the if (mom == Max ?. I wanted to avoid checking this condition in every loop again because of branch prediction and possible performance.

During one call of this function, the outcome of the outer condition will always be the same though (mom does not change at all). Therefore the actual performance penalty might be so small that it is barely measurable. Choosing the faster code would need very careful benchmarks in this case. Therefore, either variant is probably good enough.

Anyway, out of intuition I wanted to avoid as many branches as possible. To avoid the mom == Max condition inside the loop, I quickly analyzed in my head that min == bitinvert(max), at least for unsigned numbers. And since you defined the byte type as unsigned char, it seemed a good fit. Plus, I wanted to try out this bitmasking idea since I had not seen it anywhere else before.

Finding the idea of using a bitmask to convert between min and max algorithms is something that probably comes with experience, I would be very surprised if any programming beginner would find this by themselves. Even understanding it is difficult enough. For more crazy stuff, have a look at the book Hacker's Delight.

One nice last thought about this bitmasking technique is that I think it can be extended even further to also allow MinSigned (mask 0b1000_0000) and MaxSigned (mask 0b0111_1111). There might also be even stranger use cases like mask 0b0011_0000 to find the smallest ASCII digit.

Things that the bitmasking technique cannot do are:

find the first uppercase letter from the alphabet (that is, 'A' if it exists somewhere, otherwise 'B', otherwise 'C'). This is because there are 26 uppercase letters, and the code point of the 'A' is U+0041, which in binary is 0100_0001. Using this as the bitmask, the smallest bytes would be 'A@CBEDHG', in this order.
find the largest ASCII digit, for similar reasons.


Ok, the first thing is that the names are not at all descriptive. disfun? What does that mean? Maybe something like generateReport? What are the purposes of String1 and String2? Why does String2 get printed before String1? Why does String1 not get printed at all when String3 is "C"? Also, python variables tend to begin with a lowercase letter, not an uppercase letter.

As to shortening the function, there's a rule of thumb called DRY: Don't Repeat Yourself. If you see the same code written several times, extract it into a function.

Here's a first shot at improving your code:

def __printBannerEdge(char, width):
    assert len(char) == 1
    print char * width

def __printBannerContent(char, width, content):
    assert len(char) == 1
    print char * 2, ('{0:^' + str(width - 6) + '}').format(content), char * 2

def __printBanner(char, width, lines):
    __printBannerEdge(char, width)
    for line in lines:
        __printBannerContent(char, width, line)
    __printBannerEdge(char, width)

def generateReport(content, title, selection):
    if selection == "A" or selection == "B":
        if selection == "B":
            __printBannerEdge('#', 54)
        __printBannerContent('#', 54, '')
        __printBannerContent('#', 54, title)
        __printBannerContent('#', 54, content)
        __printBannerContent('#', 54, '')
        __printBannerEdge('#', 54)

    elif selection == "C":
        __printBanner('-', 40, [title])

    elif selection == 'D':
        __printBanner('#', 54, [' * '.join(title), content])

    elif selection == 'E':
        __printBanner('#', 54, [title, content])

Some commentary:

The names I use (content, title, selection) are guess from context. There may be better names possible.
I'm a little confused as to why the output is so different for each report type. I'm guessing that selection is set by user input from a menu of some sort? You should separate the function that parses user input from this function, and have 5 separate functions (eg generatePartList).
One of the oddest thing about the function as written is that sometimes title is a single string, and sometimes it seems to be a list of strings (or maybe the join statement is meant to "bold" the title name?).
Better than printing in these functions is to return a string, and let the caller print the string (that makes it easier to write your output to a file instead of standard output, for instance, if you decide later that's something you want to do).


Cool python exercise! And a nice job.
I'm going to assume you are using python3. There are some points of improvement:

Multiline strings: In python you can create a string with more than one line using """ so you an put all of the printing in the beginning of your program in one print statement.
Pathlib: There is a library called pathlib which allows you to create multi platform path objects. Try to use it instead of concatenating strings yourself.
Constants in capitals: Is access_rights a constant? Write it in capital letters if so (It's a normal naming convention).
String formatting: You can use fstrings instead of format, it looks much nicer and more readable (to me). So "Downloading images to {}{}.".format(path,x) can now be f"Downloading images to {path}{x}.".
List comprehension: you can create the yz_array using list comprehension, it is a bit complicated but it will look like: [2 ** x for x in range(8)] - will get the same result!
Meaningful variable names: instead of x even use x_index.
Meaningful exceptions: Using Exception as e you can print the system error, giving the user more information.
Logical error: Notice that even if there was an error in the file creation process you will still print "Successfully filled folder" which is not true. Add a boolean variable to prevent that.
Multi Platform compatibility: You are probably using windows, but on other os your code will crush, by adding another try - catch you can prevent that. You can also try and find the os and create a matching case for windows.

Maybe:

Functions: You can use functions to avoid the indentations.
Spaces: According to PEP8 you should add a space after a , and you should not add space after a print statement.
Use type annotation: (Last but this one is my personal favourite) When your code gets complicated and you start to use function - Do not forget to read about type annotations.

Overall - greate code, nicely written.
Good luck and Have fun!


So first, as CAD97 rightly points out, if you simply define your protocol as identically matching the methods from UserDefaults you intend to use, then making UserDefaults conform to the protocol is much simpler:

extension UserDefaults: UserDefaultsProtocol {}

And there ya go, done.  Having to write less code is nice.  What's even nicer here is that we're doing all of this presumably for the sake of testing.  If you're testing right, you're probably looking at your code coverage numbers.  Using this approach, we don't have rogue uncovered lines in the extension that we're not going to hit because we're using the mock.

I'm not a huge fan of the name you've picked for the protocol.  Maybe it's not so bad... and it does largely depend on what you're actually using user defaults for.  But more generally, user defaults is just a key-value store for persisting information between sessions.  There are other key-value store options as well though.  And I believe you'd be better served by making your protocol more generally match more of the options.  The advantage gained here is that swapping from user defaults to key chain to any other system because pretty effortless.

I do not like the UserDefaultsController at all.  You've added an extra, unnecessary layer of complications here.  It's a singleton god class... and it means that if you set up your whole app today to use user defaults, the option to change just parts of it over to a different option aren't as easy.

There's a better way.

Your current approach looks like this:

func fooBarFunc() -> String? {
    return UserDefaultsController.shared.object(forKey: "fooBarKey") as? String
}

However, we could alternatively do something more like this...

func fooBarFunc(from storage: UserDefaultsProtocol = UserDefaults.standard) -> String? {
    return storage.object(forKey: "fooBarKey") as? String
}

Both of these are still called the same:

let value = fooBarFunc()

Because in the second implementation, we pass UserDefaults.standard as the default argument for the storage to pull from.  However, when we want to test this method, we create a different object and pass that in:

let mockStorage = MockStorage(withValue: "expectedValue")
let value = fooBarFunc(from: mockStorage)
XCTAssertEqual(value, "expectedValue")

And alternative, the storage could be pass in to an object's constructor:

init(storage: UserDefaultsProtocol = UserDefaults.standard)

In the main target, we don't pass anything for this argument and just let it default to the standard user defaults.  But again in tests, we implement a mock class that conforms to the protocol and initialize our class with that mock.

This is called dependency injection.  Instead of relying on a god class living somewhere in our code base (and increasing the coupling of our whole app), we pass dependencies into the code that relies on these dependencies.  We use a protocol to keep our code base tightly aligned, but passing objects in, we keep coupling relatively low.  If one class suddenly starts needing something a little different, we can just start passing it something different instead of having to touch the code in the god class... which every other corner of our code base touches... potentially introducing tons of regressions.


I can't tell from the code sample where the loop is being called from. It seems like it's in the update method, in which case you're updating the entire map of rectangles with every change? This would be very slow. In general when dealing with GDI, GDI+, and WPF, you want to avoid doing any unnecessary work, since it is just slow.

Only update the rectangles that have actually changed between update calls.

You can do this by having a separate map initialisation function that sets up all the rectangles once, and an update function which takes a list of cell references for cells that have changed, and updates just the colour of those cells.


You should use functions and better datatypes.

You need to know how many; butter, egg, flour and sugar, bags you need, which come in large, medium and small sizes.
And so wrote the same while loop four times.
Which is perfect for a function!

Instead you should make a function if we keep it simple and have it work the way you have it now,
then you can use a function like trickle_groups(amount: int, large: int, medium: int, small: int) -> tuple<int, int, int>.
Which would get you the function:

def trickle_groups(amount, large, medium, small):
    large_bags, medium_bags, small_bags = 0, 0, 0
    while amount > 0:
        if amount > large:
            amount -= large
            large_bags += 1
        elif amount > medium:
            amount -= medium
            medium_bags += 1
        elif amount > small:
            amount -= small
            small_bags += 1
        else:
            small_bags += 1
            amount = 0
    return (large_bags, medium_bags, small_bags)

We can then use it on all the different ingredients:

butter_bags = trickle_groups(total[0], [500, 250, 125])
egg_bags    = trickle_groups(total[1], [12, 10, 6])
flour_bags  = trickle_groups(total[2], [750, 500, 250])
sugar_bags  = trickle_groups(total[3], [600, 400, 200])

Bam almost all your code is gone!
After this you should want to do the same with your print statements.
Make a format for the three duplicate print lines, and put it in a function.

DISPLAY_FORMAT = '''\
{1} large bags of {0} needed.
{2} medium bags of {0} needed.
{3} small bags of {0} needed.'''

def display_bags(type, bags):
    print(DISPLAY_FORMAT.format(type, *bags))

# ...

print("Purchases needed: ")
display_bags('butter', butter_bags)
display_bags('flour', flour_bags)
display_bags('sugar', sugar_bags)
display_bags('eggs', egg_bags)

After this I'd recommend that you rearrange the way that you go from input to output.
Instead of manually looping, you can instead actually use a for loop.
To do this you need to make a lists to loop through, so ingredients and bag sizes.
After this you'd want to actually loop through them, with zip you can quite simply.
And then you want to put it in a function main, which you call if you're in the main function:

INGREDIENTS = ['butter', 'egg', 'flour', 'sugar']
CUPCAKE = [4, 0.1, 12, 14]
LEMON = [80, 4.5, 240, 300]
BAG_SIZES = [
    [500, 250, 125],
    [12, 10, 6],
    [750, 500, 250],
    [600, 400, 200],
]

def main():
    cup_in = int(input('How many cupcakes would you like? '))
    lem_in = int(input('How much lemon cake would you like? '))

    amount_cup = [x*cup_in for x in CUPCAKE]
    amount_lemon = [x*lem_in for x in LEMON]
    total = [x + y for x, y in zip(amount_cup, amount_lemon)]
    bags = [trickle_groups(a, b) for a, b in zip(total, BAG_SIZES)]
    print("Purchases needed: ")
    for ingredient, bag in zip(INGREDIENTS, bags):
        display_bags(ingredient, bags)

if __name__ == '__main__':
    main()

You can also optimize trickle_groups, rather than looping until the amount is zero, you can instead find the amount of large bags you need, and the remainder with divmod.
Where you get the amount you need with the division, and you get the amount left with the modulo part.
I change it also to take any amount of bags, and so you should be able to get:

def trickle_groups(amount, bag_sizes):
    bag_sizes = list(bag_sizes)
    bag_sizes.sort(reverse=True)
    amount_bags = []
    for bag in bag_sizes:
        n, amount = divmod(amount, bag)
        amount_bags.append(n)
    if amount:
        amount_bags[-1] += 1
    return tuple(amount_bags)


I do stuff like that myself sometimes, but I'm not proud of it.

Strictly speaking it is the controller's responsibility to set up the new record. The controller should be be the sole source of whatever (primary) data the view needs. Instantiating a new record is business logic, not presentation logic.

In a similar vein, if we consider a simple blogging app, it'd be fine for the view to access secondary data, like:

<% @post.comments.each do |comment| %>

But I think you'd agree that it absolutely shouldn't be doing something like:

<% Post.find(params[:id]).comments.each do |comment %> <!-- please don't do this -->

Your code basically the same as the latter of those two examples. Although it seems less extreme, in both cases the view is doing the job of the controller.

Also: Assuming a typical Rails setup, your form is in its own partial, and you include it in both the edit and new views when rendering. Now, when editing, the @event_submission variable absolutely should exist - it should be set by the controller. It makes no sense for the form to create it. And when one "invocation" of the form requires it, it makes no sense for the other invocation to make it optional. So basically, the form requires an @event_submission variable - doesn't matter if it's new or not.

Letting for form instantiate the record also opens the door for hidden bugs and unintended behavior. If you navigate to /event_submissions/123/edit (or whatever the path is), but the controller somehow doesn't set the @event_submission to be edited, the form will just create its own. This means that:

You won't get an error (though you should!) because the form absorbed it. Your tests may even pass.
The form will be blank, which it shouldn't be when editing.
And since it's inferred that a form for a new record should point to the create action, if you submit the form, you're creating a new record when all you wanted was to edit one.

Everything works, there are no errors, but it's not at all doing what it's supposed to. Have fun debugging :)

Besides, your code works for HTML via ERB. But if you want a different format, or use a different templating language, you may not get away with it anymore. Since you'll then have to do the initialization in the controller anyway, you might as always do that.


put seems quite broken, which in turn breaks get.

From the documentation,

  V put(K key, V value)

  Associates the specified value with the specified key in this map
  (optional operation). If the map previously contained a mapping for
  the key, the old value is replaced by the specified value. (A map m is
  said to contain a mapping for a key k if and only if m.containsKey(k)
  would return true.)

  Returns:
  the previous value associated with key, or null if there was no mapping for key. (A null return can also indicate that the map previously associated null with key, if the implementation supports null values.)

Let's take this code

  myMap.put("x", 1);
  int oldValue = myMap.put("x", 2);
  System.out.printf("%d %d%n", oldValue, myMap.get("x"));

I would expect it to print 1 2, but it prints 3 3.