Nesting "for of" inside of a "forEach" I am new to JavaScript so I am not sure if it's bad practice to nest a for loops inside of each other.

This script simply takes a WordPress HTML menu and restructures it into a format better suited to my requirements.

Below you can see the original HTML, the JS and the new HTML output by the JS.

The nested "for of" loop checks the deepest anchor tags to see if the href matched the window and gives it a "is-active" class if it does. Should this "for of" loop be in its own function or is the nesting okay?

Is there a better way to write the JS?

Original HTML:

      Label 1

link

link

link

link

link

      Label 2

link

link

link

link

link

JS

const menuWrapper = document.querySelector(`aside.menu`);
const badMenu = document.querySelector(`.menu-list`);
const badMenuItems = document.querySelectorAll(`.menu-label`);

function fixMenus(menu, items) {
  // Remove old menu
  menu.remove();
  let fixedMenu = ``;

  // Iterate through and build new menu
    items.forEach(x => {
        const labels = x.children[0].innerHTML;
    const subMenus = x.children[1];
    const subItems = x.children[1].children;

    //Set "is-active" to anchor tag
    for (let i of subItems) {
      if (i.firstElementChild.href === window.location.href) {
        i.firstElementChild.classList.add(`is-active`)
      }
    }

    //Build new menu
        fixedMenu += `

${labels}

        ${subMenus.innerHTML}

    `;
  });

  return fixedMenu;
}

const sideMenus = fixMenus(badMenu, badMenuItems);
menuWrapper.insertAdjacentHTML(`beforeend`, sideMenus);

NEW HTML (rebuild, returned from the JS)

Label 1

link

link

link

link

link

Label 2

link

link

link

link

link

Is there a better way to write the JS?

Yes there is.
First off, lets face it, your function fixMenus looks quite complex and a double for loop in one function is usually a big "no no" sign. Any company with a coding standard would want this to be different.
You have a double for loop with some conditions and some HTML building. That's a lot for one function and it looks hard to test accurately.
Your function does the following things

Loops through each menu
Destructures some data
Loops through a sublist to apply a class
Applies the new menu to the variable

So instead of having one function handle all this, let's break it up.
Make small simple functions that are easy to test
The concept of Test Driven Development (TDD) forces you to write code that is:

Easy to understand
Easy to test

This by itself already forces you to write better code and smaller functions.
You're probably not doing TDD, but writing code as if you were is already a great step!
Focus on the simple tasks

fixMenus should call generateMenus and ultimately generateMenu
Use Array#map and Array#join instead of forEach
create a function to update the actives
create a function to generate your html string

function updateActives(subItems){
   for (let i of subItems) {
      if (i.firstElementChild.href === window.location.href) {
        i.firstElementChild.classList.add(`is-active`);
      }
   }
}

function createMenu(labels, subMenus){
  return `

${labels}

       ${subMenus}

  `
}

function generateMenu(item){
  const {children:[elem1, elem2]} = item;
  updateActives(elem2.children);
  return createMenu(elem1.innerHTML, elem2.innerHTML);
}

function generateMenus(items){
  return items.map(item => generateMenu(item)).join("");
}

function fixMenus(menu, items){
  menu.remove();
  return generateMenus(items);
}


Java PrintWriter not printing data to file This program is compiling just fine, and the variables and such seem to be passing through.  However, when I open the text file there is no data.  Can anyone point me in the right direction or nudge my brain into the right process thought?

package dvdlogger;
import java.util.*;
import java.io.*;

public class DVDLogger {
public DVDLogger() throws IOException
{
    String title = null;
    double price = 0;
    storeDVDInfo(title, price);
}
private void storeDVDInfo(String title, double price) throws IOException
{
    File path = new File("C:\\users\\crazy\\desktop\\dvd.txt");
    PrintWriter output = new PrintWriter(path);
    output.printf("%1$s %2$.2f", title, price);
}

public static void main(String[] args) throws IOException{
    DVDLogger dvd;
    String title;
    double price;
    try (Scanner read = new Scanner(System.in)) {
        dvd = new DVDLogger();
        System.out.printf("%nPlease enter the title of the DVD: ");
        title = read.nextLine();
        System.out.printf("%nPlease enter the price of the DVD: ");
        price = read.nextDouble();
    }
    dvd.storeDVDInfo(title, price);
}
}

With any Java Writer or Stream... YOU MUST PROPERLY FLUSH AND CLOSE it.

The documentation is not fantastically clear... but:

when you exit the method, you must ensure that the file is flushed and closed. you are exiting your program before the data is written to disk.

Always use the following pattern when doing IO (if you are using Java7) :

try (PrintWriter output = new PrintWriter(path)) {
    output.printf("%1$s %2$.2f", title, price);
    output.flush();
}

This is a 'try-with-resource' block.


Searching for an object with spectific property, using user input I have just started my adventure with C#. Below you can find a code written for my school assigment.

The homework task had 3 major elements:

building a class book
creating a metod for changing the static genre value and
creating a way to find specific object by properties.

All those elements are included.

I hope that more experienced members of community will have a short look at my code and share some tips and/or good practices, so I may improve.

class Book
{
    public string title { get; set; }
    public string Author { get; set; }
    public string publisher { get; set; }
    public int price { get; set; }
    public static string genre;

    public override string ToString()
    {
        string output = String.Format("Title :{0} \n Author :{1} \n Publisher :{2} \n Price {3}", this.title, this.Author, this.publisher, this.price);
        return output;
    }

    public static List<Book> myList = new List<Book>();
    private static void changegenre()
    {
        string mygenre = "Fantasy";
        genre = mygenre;
    }

    static void Main(string[] args)
    {
        changegenre();
        myList.Add(new Book() { title = "Hobbit", Author = "J.R.R Tolkien", publisher = "U & G", price = 34, });
        myList.Add(new Book() { title = "Two Towers", Author = "J.R.R Tolkien", publisher = "U & G", price = 55 });
        myList.Add(new Book() { title = "Opowiesci z meekhanskiego pogranicza", Author = "Robert M. Wegner", publisher = "Powergraph", price = 10 });
        myList.Add(new Book() { title = "A Dance with Dragons", Author = "G.R.R Martin", publisher = "Voyager Books", price = 25 });
        Console.WindowWidth = 95;
        Console.BackgroundColor = ConsoleColor.DarkMagenta;
        Console.WriteLine(" --------Welcome to our bookstore!-------");

        do
        {

            Console.BackgroundColor = ConsoleColor.Black;
            Console.WriteLine("Please write: \r\n a = if you want to search book by author, \r\n p = if you want to search book by price, \r\n pub - if you want to search by publisher or \r\n t- if you want to search by title");
            string input = Console.ReadLine();
            switch (input)
            {
                case "a":
                    searchbyauthor();
                    break;

                case "p":
                    searchbyprice();
                    break;
                case "pub":
                    searchbypublisher();
                    break;
                case "t":
                    searchbytitle();
                    break;

            }

            Console.WriteLine("Do you want to search again?");
        } while (Console.ReadLine().ToUpper() == "YES");
    }

    private static void searchbytitle()
    {
        Console.WriteLine("Please, write book's title!");
        string input = Console.ReadLine();
        foreach (Book result in myList.Where(x => x.title == input).ToList())
        {
            Console.WriteLine(result);

        }
    }

    private static void searchbypublisher()
    {
        Console.WriteLine("Please, write publisher's name!");
        string input = Console.ReadLine();
        foreach (Book result in myList.Where(x => x.publisher == input).ToList())
        {
            Console.WriteLine(result);

        }
    }

    private static void searchbyprice()
    {
        Console.WriteLine("Please, give me the maximum book price");
        string input = Console.ReadLine();
        int maxprice = Convert.ToInt32(input);
        foreach (Book result in myList.Where(x => x.price <= maxprice).ToList())
        {
            Console.WriteLine(result);
        }
    }

    private static void searchbyauthor()

    {
        Console.WriteLine("Please, write author's name!");
        string input = Console.ReadLine();
        foreach (Book result in myList.Where(x => x.Author == input).ToList())
        {
            Console.WriteLine(result);

        }
    }

}

Always nice to see a student eager to learn.

Class Book

Your should try to always use an access modifier, so this should be

public class Book

Other issues:

Public properties in C#, should begin with a capital letter.
Price should be a Decimal (used for currency or monetary values) instead
of a whole number.
Genre should rightfully be a property to a Book, and not static to the entire class.  Changing the genre then becomes as easy as updating the property.
myList does not belong in the Book class. It should not be static and should be moved to Main.
ToString() does not need to refer this..  Nor is the output needed.  Instead just return the string.Format.

Search Methods

The names should use PascalCasing, example SearchByPublisher, SearchByPrice.

There is no reason for the trailing .ToList() inside each respective foreach.  The Where has already returned an enumerable collection that you will iterate over.

For comparing strings, you have no regard for case.  Thus "HOBBIT" and "hobbit" would not match "Hobbit".  You may want to allow for ignoring the case.  Another consideration would be on matching a partial string.  Consider if the user enters "dragons" at the console that maybe you want to return "A Dance with Dragons".

Going Further

As a beginner, be sure you grasp what you see so far.  Then go a step further and add a Books class that implements List<Book>.  Now the search methods could be moved inside the Books class.  In the name of Separation of Concerns, you don't want the searches to also write to the console.  Instead, the search should be concerned only with searching.  That would mean each respective search would return an IEnumerable, or really just the results of the Where( ... ).


Small "Pyramid Escape" text-based game I'm extremely amateur when it comes to C++ in general, but I wanted to make a little "Pyramid Escape" text based game. I'm not done whatsoever but before I continue I want to see what I could possible do differently.

//
//  main.cpp
//  TextFiles
//
//  Created by Neal Carico on 3/12/15.
//  Copyright (c) 2015 Nedev. All rights reserved.
//

#include <iostream>
#include <cstdlib>

using namespace std;

int main(int argc, const char * argv[]) {

string start;
string restart;
string dead;
int turn1;
int turn2;
int turn3;
int turn4;
string decision1;
string decision2;
string decision3;
string decision4;
string decision5;
int random1;
int random2;
int random3;

while(restart != "dontrestart" || dead != "false"){
cout << "Welcome to the worlds largest pyramid. . . Type 'start' to enter or 'leave' to walk away..." << endl;
cin >> start;

///The Start
if (start == "start"){

///The main part of the game.

    cout << "Oh no! The pyramid collapsed trapping you in. You are the only one alive. . ." << endl;

    int score=0;

    cout << "Okay. So you have two options here. . .Left(1) or right(2)?" << endl;
    cin >> turn1;
    if(turn1 == 2){
        cout << "BAM! The roof collapsed over you and you die. Sad stuff" << endl;
        cout << "Thank you for playing. Your score was " << score << endl;
        exit(1);
    }
    cout << "You took the left path. And then BAM! The path to the right collapsed... lucky you." << endl;
    score++;
    cout << "There gold on the ground.. do you pick it up?[y/n]" << endl;
    cin >> decision1;

    if(decision1 == "y"){
        cout << "You greedy bastard you. Lucky I like the irony and am not going to kill you in this instance... You find 10 gold." << endl;
        score = score + 10;
    }

    cout << "You start heading forward again and come to another turn. But this time there are actually four options... :)" << endl;
    random1 = rand() % 4 +1;
    cout << "Your options are as follows. 1) Up to right 2)Down to right 3)Up to left and 4)Down to left. This one changes everytime ;). The random number is " << random1 << endl;
    cin >> turn2;
    if (turn2 == random1) {
        cout << "BAM! The roof collapsed over you and you die. Sad stuff" << endl;
        cout << "Thank you for playing. Your score was " << score << endl;
        exit(1);
    }
    cout << "Sweet. You made it. There's a point." << endl;
    score++;

    cout << "Okay you start walking down the tunnel and something is moving in the distance. Do you approach it or head back to one of the other tunnels?(continue/return" << endl;
    cin >> decision2;
    if(decision2 == "continue"){
        cout << "Yup, I'm not nice enough to give you a break 2 times in a row. Dead." << endl;
        cout << "BLAAAHHHGGGG!! The figure is a crazed mummy who attacks you and you die. Sad stuff" << endl;
        cout << "Thank you for playing. Your score was " << score << endl;
        exit(1);
    }
    cout << "There is a screeching sound as you turn around." << endl;
    score++;

///End of game stuff

}else if(start == "leave"){
    cout << "You walked away and heard a huge BOOM and saw the pyramid collapsed trapping the tour group in. You are" <<
    " one lucky person. Sure you lived, but you didn't even play the game. How lame.You can type 'restart' to try again, but you're always going to be a loser to me." << endl;
}else{
    cout << "Error. Please type restart or 'dontrestart' (notice there are no spaces) to exit the game..." << endl;
}
    cin >> restart;
    if (restart !="restart"){
        exit(1);
    }

}
return 0;

}

First, don't use using namespace std;, or any other namespace, for that matter. This can cause you have load functions with the same name, which can cause problems. This is discussed in more detail here.

Second, you have a ton of variables you never use, including:

int turn3;
int turn4;
string decision3;
string decision4;
string decision5;
int random2;
int random3;

Third, you should use move functions.  One candidate for a function is asking whether to restart the game.

Fourth, I would use bools for dead and restart.  You will need to convert the input for restart, but it better represents what is happening.

Fifth, you should consistently indent your code at 4 spaces per level.  You do this for the most part, but you do not do this in main().


Java Brainfuck Interpreter Method Due to the fact that nobody's ever posted this before (sarcasm) I've made a brainfuck interpreter in java. The interpreter fits in a single method, and is designed to be readable, fast, and concise, so please review on those things. I have tested it to confirm that it works.

/**
 * Interpret program as a brainfuck program with the given standard input
 * and output.
 *
 * @throws IOException
 *             if thrown by stdin or stdout
 */
public static void brainfuck(String program, OutputStream stdout, InputStream stdin) throws IOException {
    int[] commands = program.chars().filter(c -> "><+-./[]".indexOf(c) != -1).toArray();
    int commandPointer = 0;
    int[] memory = new int[30_000];
    int memoryPointer = 0;
    while (commandPointer < commands.length) {
        switch (commands[commandPointer]) {
        case '>':
            memoryPointer++;
            if (memoryPointer >= memory.length)
                memoryPointer = 0;
            break;
        case '<':
            memoryPointer--;
            if (memoryPointer < 0)
                memoryPointer = memory.length - 1;
            break;
        case '+':
            memory[memoryPointer]++;
            if (memory[memoryPointer] > 255)
                memory[memoryPointer] = 0;
            break;
        case '-':
            memory[memoryPointer]--;
            if (memory[memoryPointer] < 0)
                memory[memoryPointer] = 255;
            break;
        case '.':
            stdout.write(memory[memoryPointer]);
            break;
        case ',':
            memory[memoryPointer] = stdin.read();
            memory[memoryPointer] %= 256;
            break;
        case '[':
            if (memory[memoryPointer] == 0) {
                int depth = 1;
                while (depth > 0) {
                    commandPointer++;
                    if (commands[commandPointer] == '[')
                        depth++;
                    else if (commands[commandPointer] == ']')
                        depth--;
                }
            }
            break;
        case ']':
            if (memory[memoryPointer] != 0) {
                int depth = 1;
                while (depth > 0) {
                    commandPointer--;
                    if (commands[commandPointer] == ']')
                        depth++;
                    else if (commands[commandPointer] == '[')
                        depth--;
                }
            }
            break;
        }
        commandPointer++;
    }
}

You already reached two of your goals. The interpreter is very readable and very concise. It is pretty fast but could easily be made faster, but then the code would become longer and more complicated.

If you want the interpreter to stay readable, leave it as-is. Maybe document the wrap-around semantics in the Javadoc.

One thing you could change is for the , command to just be memory[memoryPointer] = stdin.read() & 0xFF;. This is more common than % 256, since this is about bit msnipulation, not about arithmetics.


shared_ptr alternative with no dependencies An alternative to shared_ptr to minimize compile time. Intrusive. Reference count changes are not thread safe.

(#include<memory> pulls in 17k lines of code)

This satisfies my needs, but I'm curious how it could be improved (without adding any dependencies).

It's not particularly good at handling incomplete types (ref<T>::get cannot be used on an incomplete type). Is it possible to improve that?

// Derive from this to be refcounted.
struct refcount {
  int rc = 0;
  virtual ~refcount() { }
};

void queued_delete(refcount*);

template<class T>
class ref {

public:

  ref() { }

  ref(T* obj) : obj(obj) {
    if(obj) {
      obj->rc++;
    }
  }

  ~ref() {
    if(obj) {
      if(--(obj->rc) == 0) {
        queued_delete(obj);
      }
    }
  }

  ref(const ref& r) {
    if(r.obj) {
      obj = r.obj;
      obj->rc++;
    }
  }

  template<class T2>
  ref(const ref<T2>& r) {
    if(r.get()) {
      obj = r.get();
      obj->rc++;
    }
  }

  ref& operator=(const ref& r) {
    ref(r).swap(*this);
    return *this;
  }

  template<class T2>
  ref& operator=(const ref<T2>& r) {
    ref(r).swap(*this);
    return *this;
  }

  T* operator->() { return static_cast<T*>(obj); }
  const T* operator->() const { return static_cast<const T*>(obj); }
  T& operator*() { return *get(); }
  T& operator*() const { return *get(); }

  T* get() const { return static_cast<T*>(obj); }

  unsigned long id() const { return (unsigned long) obj; }

  void swap(ref& p) {
    auto tmp = obj;
    obj = p.obj;
    p.obj = tmp;
  }

  explicit operator bool() const {
    return obj;
  }

  bool operator==(const ref& other) const {
    return obj == other.obj;
  }

  bool operator!=(const ref& other) const {
    return obj != other.obj;
  }

  template<class T2>
  ref<T2> cast() {
    return ref<T2>(dynamic_cast<T2*>(obj));
  }

private:
  refcount *obj = nullptr;
};

Optional deletion which can handle deep nesting (can also do this with shared_ptr using a custom deleter):

// Ensure deeply nested data structures can
// be deleted without running out of stack.
void queued_delete(refcount* p) {

  static bool deleting = false;
  static std::vector<refcount*> stack;

  stack.push_back(p);

  if(!deleting) {
    deleting = true;
    while(stack.size()) {
      auto top = stack.back();
      stack.pop_back();
      delete top;
    }
    deleting = false;
  }
}

You didn't post a complete header file (no include-guard/#pragma once), so it's unclear whether you also omitted a namespace Taylor { somewhere in there. But you should definitely use a namespace around a name as common as ref.

  virtual ~refcount() { }

Nit: You could use = default; instead of { } here, and you might get slightly better codegen on some compilers. (Virtual destructors are never actually "trivial," unfortunately, but getting the compiler to write this quasi-trivial code for you can't hurt.)

I notice that refcount->rc is public, not private. Was that a design decision?

It occurs to me that the proper name for what you're calling refcount is actually refcounted. The refcount, physically, is the data member rc. A type which derives from your base class is a refcounted type.

ref(T* obj) : obj(obj) {
  if(obj) {
    obj->rc++;
  }
}

I would write this as

explicit ref(T *obj) : obj_(obj) {
    if (obj_ != nullptr) {
        obj_->rc += 1;
    }
}

Notice the whitespace edits, the use of some kind of sigil for data members (to avoid having two different variables named obj in scope), my preference for += 1 when the increment is a stand-alone statement, the explicit comparison against nullptr in place of contextual-conversion-to-bool, and the addition of explicit. Non-explicit constructors permit implicit conversions, e.g.

struct Widget : public refcount { int data = 42; };

void print_data_of(ref<Widget> x) { std::cout << x->data << "\n"; }

int main() {
    Widget *p = new Widget;
    print_data_of(p);
    delete p;  // OOPS! Double delete!
}

It would be better (IMO of course) if this code did not compile.

template<class T2>
ref(const ref<T2>& r) {
  if(r.get()) {
    obj = r.get();
    obj->rc++;
  }
}

This is extremely sketchy. Consider:

struct Widget : public refcount { int data = 42; };
struct Gadget : public refcount { int x = -1; int data = 42; };
int main() {
    ref<Widget> w = new Widget;  // OK...
    ref<Gadget> v = w;           // ...sketchy...
    std::cout << v->data << "\n";  // OOPS! Prints "-1", not "42"
}

This constructor should be either completely removed, or else constrained (using enable_if, C++2a requires, or some other trickery) so that it participates in overload resolution only when T2* would be convertible to T*.

One easy way to mostly-fix this would be to simply add an assertion:

template<class T2>
ref(const ref<T2>& r) {
    static_assert(std::is_convertible_v<T2*, T*>);
    if (r != nullptr) {
        obj_ = r.get();
        obj_->rc += 1;
    }
}

Here we're lying to the library (e.g. std::is_constructible_v<ref<Widget>, ref<Gadget>> will still be true), but at least we prevent the client programmer from accidentally writing a program like the test case above.

Another way to fix the issue would be to rely on pointer-assignment to do the check for us. Instead of refcount *obj_;, let's make our data member look like T *obj_;. Then we can write

template<class T2>
ref(const ref<T2>& r) {
    if (r != nullptr) {
        obj_ = r.obj_;  //HERE
        obj_->rc += 1;
    }
}

And then if T2* isn't convertible to T*, we'll get an error on the line marked //HERE.

Incidentally, this also solves your problem with ref<T>::get() and incomplete types.

T& operator*() { return *get(); }
T& operator*() const { return *get(); }

You don't need both versions of the function, since they do exactly the same thing. Just write the const version. (Dereferencing a pointer doesn't need to modify the pointer, remember. Const is a contract.)

T* operator->() { return static_cast<T*>(obj); }
const T* operator->() const { return static_cast<const T*>(obj); }

And in this case you've got the two versions doing different things, but that's still wrong, because they shouldn't be doing different things! Dereferencing a pointer doesn't need to modify the pointer. What you meant in both cases was simply

T* get() const { return static_cast<T*>(obj); }
T* operator->() const { return get(); }
T& operator*() const { return *get(); }

Your void swap(ref& p) should be noexcept — just like your move-constructor, which I guess you didn't write. (You should do some move semantics here!)

I recommend implementing your swap as a one-liner: std::swap(obj_, rhs.obj_);.

A member swap function will not be picked up by any standard library algorithms. If you want your swap to be actually used, you'll need to provide an ADL swap, like this:

friend void swap(ref& a, ref& b) noexcept { a.swap(b); }

Your queued_delete is interesting. It's misnamed, in that its deletions are stacked (LIFO), not queued (FIFO). I don't know if that makes a difference to performance or anything like that, in practice.

It's also thread-unsafe, which is not clear from your description/documentation. In standard C++, we can write

int main() {
    std::shared_ptr<Widget> p(new Widget);
    std::thread([q = p]() {
        q = nullptr;
    }).detach();
    p = nullptr;
}

and be guaranteed that the writes to the refcount shared by p and q won't race with each other. Your documentation clearly states that we can't do that with your ref<Widget>. But what is surprising to me is that we also can't do the following!

int main() {
    ref<Widget> p(new Widget);
    std::thread([]() {
        ref<Gadget> q(new Gadget);
        q = nullptr;
    }).detach();
    p = nullptr;
}

Here, p.obj->rc and q.obj->rc are completely different objects, so there's no race there; but then they each call into queued_delete and try to write to stack, and those writes race with each other. So your ref is completely unsafe for use within a multi-threaded environment, even if you never share any objects between threads.

In theory I guess you could "fix" this by replacing the storage class static with thread_local everywhere it appears; but, please don't do that.


Python task runner with asyncio I'm trying to a write super simple task runner with asyncio.

Basically, my goal is to run 100 requests to google as tasks, whilst also being be able to handle tasks being added at a later time.

from abc import ABC, abstractmethod
import asyncio
import socket
import time

import aiohttp
import requests

class MyScheduler:
    def __init__(self, wait=2):
        self.work = []
        self.wait = wait

    def set_initial_callback(self, callback, **kwargs):
        self.initial_callback = callback
        self.initial_callback_args = kwargs

    async def add_task(self, callback, **kwargs):
        task = asyncio.create_task(callback(**kwargs))
        self.work.append(task)

    async def _run(self):
        while self.work:
            task = self.work.pop()
            await  task
            if len(self.work) == 0:
                await asyncio.sleep(self.wait)

    async def set_things_up(self, callback, **kwargs):
        task = asyncio.create_task(callback(**kwargs))
        self.work.append(task)
        await self._run()

    def go(self):
        asyncio.run(self.set_things_up(self.initial_callback, **self.initial_callback_args))

async def google(n):
    if n == 100:
        return None
    await s.add_task(google, n=n + 1)

    async with aiohttp.ClientSession() as session:
        async with session.get('http://h...content-available-to-author-only...n.org/get') as resp:
            print(resp.status)

t = time.time()
s = MyScheduler(wait=1)
s.set_initial_callback(google, n=1)
s.go()
print(time.time() - t)

I benchmarked this against sequentally running requests, and I did see a massive speed up. It's still super rough, but I'd love some pointers on how  I could improve my code in terms of readability/exploiting async stuff better.

I actually just started learning asyncio a couple days ago, so I won't be able to comment too deeply. I do see a few things though:

Disregarding asyncio for a sec, I think google could be set up better. You have the base case of the recursion as n == 100, and are incrementing n in each recursive call. To easily allow the caller to decide how many time to run, I'd reverse how n is being handled. I would decrement it each call, and set the base case as n <= 0. With how you have it now, say the caller wanted it to run 1000 times, they would need to call it as

google(-900)

which is a little wonky. I'd change the first bit to:

async def google(n):
    if n <= 0:
        return None

    await s.add_task(google, n=n - 1)

. . .

I'm not sure recursion is the cleanest tool for the job here. I'm also not sure entirely why you're using a job queue or why you're using a elaborate class here unless the goal is to be able to handle jobs being added at a later time.

If your goal is just to initiate many requests and wait on them at the same time, you could just gather them:

import aiohttp
import asyncio as a

# google no longer cares about how many times it runs
# That is arguably beyond the responsibilities of a function intended to make requests
async def google():
    async with aiohttp.ClientSession() as session:
        async with session.get('http://h...content-available-to-author-only...n.org/get') as resp:
            print(resp.status)

async def start_requests(n_requests: int):
    routines = [google() for _ in range(n_requests)]  # Create a list of reqeust-making coroutines
    await a.gather(*routines)  # Unpack the routines into gather (since gather is var-arg)

Also, instead of doing timing using a single attempt and plain subtraction, it would be more accurate to use timeit:

from timeit import timeit

print("t:", timeit(lambda: a.run(start_requests(10)), number=20))  # number is the amount of tests to do

I'm assuming there's no issue using timeit for async code.


Undirected graph data structure in C# Description
A class representing an undirected graph. At the moment, it supports integer values as vertices. An example of the type of graph represented is shown in the following diagram:

It is represented internally as an array of adjacency lists. Each index of this array corresponds to a vertex number, and contains a List<int> of vertex numbers to which it is connected.
Purpose
Purely as an exercise in implementing a data structure. Implemented in C# after studying a Java-based implementation.
Code
The data structure class definition
public class UndirectedGraph
{
    // number of vertices
    private int _V;

    // number of edges
    private int _E = 0;

    // array of adjacency lists
    private List<int>[] _adj;

    /// <summary>
    /// Constructor to create a graph.
    /// </summary>
    /// <param name="v">Number of vertices</param>
    public UndirectedGraph(int v)
    {
        this._V = v;

        // create array of lists
        // initialise all lists to empty
        this._adj = new List<int>[v];
        for (int i = 0; i < this._adj.Length; i++)
            this._adj[i] = new List<int>();
    }

    // number of vertices
    public int V
    {
        get { return this._V; }
    }

    // number of edges
    public int E
    {
       get { return this._E; }
    }

    /// <summary>
    /// Add an edge to the graph.
    /// </summary>
    /// <param name="v">One vertex of the new edge.</param>
    /// <param name="w">The other vertex of the new edge.</param>
    public void addEdge(int v, int w)
    {
        // validate given node numbers
        if ((v > this._adj.Length) || (w > this._adj.Length))
            throw new ArgumentException("Invalid node number specified.");

        // add to adjacency lists
        this._adj[v].Add(w);
        this._adj[w].Add(v);
        // increment edge count
        this._E++;
    }

    /// <summary>
    /// Get an adjacency list for a vertex.
    /// </summary>
    /// <param name="v">The vertex.</param>
    /// <returns></returns>
    public List<int> getAdjacency(int v)
    {
        return this._adj[v];
    }

    /// <summary>
    /// Get a string representation of the graph's adjacency lists.
    /// </summary>
    /// <returns></returns>
    public override string ToString()
    {
        StringBuilder builder = new StringBuilder();
        builder.AppendLine(this._V + " vertices, " + this._E + " edges");
        for (int v = 0; v < this._V; v++)
        {
            builder.Append(v + ": ");
            foreach (var w in this._adj[v])
            {
                builder.Append(w + " ");
            }
            builder.AppendLine(string.Empty);
        }

        return builder.ToString();
   }
}

Sample usage. Prints out a string representation of the example graph in the diagram above to the console:
UndirectedGraph myGraph = new UndirectedGraph(7);
myGraph.addEdge(0,1);
myGraph.addEdge(0,2);
myGraph.addEdge(2,3);
myGraph.addEdge(2,4);
myGraph.addEdge(2,5);
myGraph.addEdge(5,6);
Console.Write(myGraph.ToString());

// RESULT:
7 vertices, 6 edges
0: 1 2
1: 0
2: 0 3 4 5
3: 2
4: 2
5: 2 6
6: 5

Questions

Any feedback about variable names, commenting and implementation details.
Changes, if any, to make it unit-testable.
Ways to include C# 7.0 features, if they haven't been already.
How to extend this to other data types, such as string.
Performance improvements might be interesting.

Naming
Renaming your instance variable _V and _E to nbVertices and nbEdges respectively allows you get rid of two unnecessary comments and also makes the rest of the code much clearer, especially to someone who's not too familiar with graph notation.
I'm also not a big fan of prefixing private instance variables with underscores, but that's just my personal opinion.
Similarly, you could give most of your methods' arguments much clearer names.
The standard naming convention for C# is to use PascalCase for methods & properties, and camelCase for fields, so I would rename getAdjacency() and addEdge() to GetAdjacency() and AddEdge().
Properties
    // number of vertices
    public int V
    {
        get { return this._V; }
    }

    // number of edges
    public int E
    {
       get { return this._E; }
    }

can be changed to
    // number of vertices
    public int V
    {
        get => this._V;
    }

    // number of edges
    public int E
    {
       get => this._E;
    }

which can further be simplified to
    // number of vertices
    public int V => this._V;

    // number of edges
    public int E => this._E;

You could go one step further and merge V and _V into a single property, like this :
public int V { get; private set; }

public int E { get; private set; }

and since V doesn't change after being set in the constructor, you can go even further beyond and make V into a read-only property :
public int V { get; }

This makes it so that V can only be set once in the constructor.
Comments
Personal opinion again, but I don't like comments, especially when they can be made useless by simply changing a variable or a method's name, as was the case with your declarations of _V or _E.
I also really don't like comments that just say what the code is doing, like in your addEdge() method :
public void addEdge(int v, int w)
{
    // validate given node numbers
    if ((v > this._adj.Length) || (w > this._adj.Length))
        throw new ArgumentException("Invalid node number specified.");

    // add to adjacency lists
    this._adj[v].Add(w);
    this._adj[w].Add(v);
    // increment edge count
    this._E++;
}

These comments add nothing more than redundant, distracting text that makes the code harder to read. If you see a line of code that says NbEdges++;, do you really need a comment to say // increment edge count ?
Exceptions
I'd recommend always throwing the exception that most closely fits the issue, which in this case would be an ArgumentOutOfRangeException (which is derived from ArgumentException).
Additionally, I would make the exception message more clear and give more information about what actually went wrong. Imagine being a user and seeing "Invalid node number specified."; which argument was wrong? Was it v or w? Why was the value wrong?
You could go even further and create your own exception class that inherits said exception, in order to reduce code repetition and give your exceptions meaningful names which would help your users.
if (v < 0 || v > this._adj.Length)
{
    throw new ArgumentOutOfRangeException($"Error, the value provided for 'v' should be" +
        "between 0 and {this._adj.Length - 1}\n" +
        "\tv = {v}\n");
}

if (w < 0 || w > this._adj.Length)
{
    throw new ArgumentOutOfRangeException($"Error, the value provided for 'v' should be" +
        "between 0 and {this._adj.Length - 1}\n" +
        "\tv = {v}\n");
}


Shuffling and hiding views specified by parameters In my Angular app, users click on different icons. When a user clicks on respective icon, I can show the icons gallery, as well shuffle the view.

Apart from this shuffling functionality, I need to hide whatever other gallery is already open.

For that, I've written the following function, but, I think it could use some refactoring.

$scope.galleryMenu = function ( GalleryType ) {

            var hideGallery = (function (type) {

                return {

                    galleryVideoShow : function () {

                        //closing others
                        $scope.contractorShow = false;
                        $scope.galleryPhotoShow = false;
                    },

                    contractorShow : function () {
                        //closing others
                        $scope.galleryPhotoShow = false;
                        $scope.galleryVideoShow = false;

                    },

                    galleryPhotoShow : function () {

                       //closing others
                        $scope.contractorShow = false;
                        $scope.galleryVideoShow = false;

                    }

                }

            })();

            $scope.$watch('viewProjectInfo', function ( newValue, oldValue ) {

                if(!newValue) return;

                $scope.photos = $scope.viewProjectInfo.ImagePaths;

                $scope[GalleryType] = !$scope[GalleryType];
                $scope.shownGallery =  GalleryType;

                hideGallery[GalleryType](); //manually close other open gallery

                $('html, body').animate({scrollTop:$(document).height()}, 'slow');

            });

        };

A few things that could be improved:

You have quite a lot of empty white lines, and extraneous space:

return {

    galleryVideoShow : function () {

        //closing others
        $scope.contractorShow = false;
        $scope.galleryPhotoShow = false;
    },

    contractorShow : function () {

    // ...

This block, for example, without extraneous whitespace, would look like:

return {
    galleryVideoShow: function(){
        $scope.contractorShow,
        $scope.galleryPhotoShow = false;
    },
    contractorShow: function(){

    //...

        var hideGallery = (function (type) {

            return {

                galleryVideoShow : function () {

                    //closing others
                    $scope.contractorShow = false;
                    $scope.galleryPhotoShow = false;
                },

                contractorShow : function () {
                    //closing others
                    $scope.galleryPhotoShow = false;
                    $scope.galleryVideoShow = false;

                },

                galleryPhotoShow : function () {

                   //closing others
                    $scope.contractorShow = false;
                    $scope.galleryVideoShow = false;

                }

            }

        })();

There's no reason to assign hideGallery like that.

It'd be better as a plain object.

var hideGallery = {
        galleryVideoShow: function(){
            //closing others
            $scope.contractorShow,
            $scope.galleryPhotoShow = false;
        },
        contractorShow: function(){
            $scope.galleryPhotoShow,
            $scope.galleryVideoShow = false;
        },
        galleryPhotoShow: function(){
            $scope.contractorShow,
            $scope.galleryVideoShow = false;
        }
}

Which would be better called something like GALLERY_EFFECTS, and kept as a constant.

Also note that for each function in hideGallery, the comment //closing others was present, it's only part of what the functions do, and would be better off at the root of hideGallery in a more expanded version.

       $scope.$watch('viewProjectInfo', function ( newValue, oldValue ) {

            if(!newValue) return;

            $scope.photos = $scope.viewProjectInfo.ImagePaths;

            $scope[GalleryType] = !$scope[GalleryType];
            $scope.shownGallery =  GalleryType;

            hideGallery[GalleryType](); //manually close other open gallery

            $('html, body').animate({scrollTop:$(document).height()}, 'slow');

        });

    };

Why are there so many empty lines at the bottom? (six, I counted) They just look poorly formatted, or like missing code.
You should wrap your conditionals in brackets, because untold circumstances can happen.
You should spread the .animate call over a multiple lines, as you're building an object inside the function call.

   $scope.$watch('viewProjectInfo', function(newValue, oldValue){
       if (!newValue){ return };
       $scope.photos = $scope.viewProjectInfo.ImagePaths;

       $scope[GalleryType] = !$scope[GalleryType];
       $scope.shownGallery = GalleryType;

       hideGallery[GalleryType]();

       $('html, body').animate({
           scrollTop: $(document).height()
       }, 'slow');
   });


TDD for Euler Problem #4 I am in the process of learning Java and going through the Euler problems. For Problem 4 I decided to also implement a TDD approach with JUnit.

My questions are:

Is there a better TDD approach to this problem?

I wrote out all these test cases initially. Some obviously pass and some fail initially because I'm checking a boolean. This makes it impossible to write a failing test in this fashion, should I have written only a failing scenario (say test100), then added in more as my method worked?

Being new to Java, are there obvious code organization things I am missing?
Any naming tips would be appreciated as well as I am terrible at this generally!

I have three source files:

main.java

public class main {

    public static void main(String[] args) {

        PalindromeChecker p = new PalindromeChecker();
        int largest=0;
        int product;

        for (int i = 999; i>100; i--){

            for (int j = 999; j>100; j--){
                product = i*j;
                if (p.isPalindrome(product) && product > largest) {
                    largest = product;
                }
            }
        }
        System.out.println(largest);
    }
}

PalindromeChecker.java

public class PalindromeChecker {

    public boolean isPalindrome(int input){

        String str = new Integer(input).toString();
        int len = str.length();

        for(int i = 0; i < Math.ceil(len/2.0); i++){

            if (str.charAt(i) != str.charAt(len-i-1)) {
                return false;
            }
        }

        return true;

    }
}

PalindromeCheckerTest.java

import static org.junit.Assert.*;
import org.junit.Test;

public class PalindromeCheckerTest {

    @Test
    public void test10() {
        PalindromeChecker checker = new PalindromeChecker();
        assertFalse("10 failed",checker.isPalindrome(10));
    }
    @Test
    public void test112() {
        PalindromeChecker checker = new PalindromeChecker();
        assertFalse("112 failed",checker.isPalindrome(112));
    }
    @Test
    public void test900() {
        PalindromeChecker checker = new PalindromeChecker();
        assertFalse("900 failed",checker.isPalindrome(900));
    }

    @Test
    public void test1000() {
        PalindromeChecker checker = new PalindromeChecker();
        assertFalse("1000 failed",checker.isPalindrome(1000));
    }

    @Test
    public void test909() {
        PalindromeChecker checker = new PalindromeChecker();
        assertTrue(checker.isPalindrome(909));
    }

    @Test
    public void test1001() {
        PalindromeChecker checker = new PalindromeChecker();
        assertTrue(checker.isPalindrome(1001));
    }

    @Test
    public void test11() {
        PalindromeChecker checker = new PalindromeChecker();
        assertTrue(checker.isPalindrome(11));
    }

    @Test
    public void test111() {
        PalindromeChecker checker = new PalindromeChecker();
        assertTrue(checker.isPalindrome(111));
    }

}

First of all, your PalindromeChecker does not contain any state, it is a simple input --> output operation not depending on outer factors. This makes it an ideal candidate for being a static method.

Your naming is okay overall, but I would use the name Palindromes for your class.

String str = new Integer(input).toString();

It is better to use String.valueOf here. There's no need to create an Integer object.

for (int j = 999; j>100; j--){

Technically, you're off by one here. You're missing the case when j == 100, which is also a three-digit number (the same goes for i btw).

Correct would be (also with improved spacing)

for (int j = 999; j >= 100; j--) {

So, here's how I would make your class:

public class Palindromes {

    public static boolean isPalindrome(int input) {
        String str = String.valueOf(input);
        int len = str.length();

        for(int i = 0; i < Math.ceil(len/2.0); i++) {
            if (str.charAt(i) != str.charAt(len - i - 1)) {
                return false;
            }
        }
        return true;
    }

}

All your test cases contains pretty much the same code, but with different parameters.

With the change to a static method, the test cases will look more like:

assertFalse("10 failed", Palindromes.isPalindrome(10));

This is ideal for a Parameterized test, which will make it significantly easier to add more tests.

@RunWith(Parameterized.class)
public class PalindromeTest {
    @Parameters
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][] {
                 { 10, false },
                 { 100, false },
                 { 909, true },
                 ...
           });
    }

    private final int input;

    private final boolean expectedPalindrome;

    public PalindromeTest(int input, boolean expectedPalindrome) {
        this.input = input;
        this.expectedPalindrome = expectedPalindrome;
    }

    @Test
    public void test() {
        assertEquals("Failed for " + input,
            expectedPalindrome, Palindromes.isPalindrome(input));
    }
}


dot() gets called three million times.

I changed it from:

def dot(v,w):
    return sum(v_i * w_i for v_i, w_i in zip(v,w))

to:

def dot(v,w):
    return (v[0] * w[0]) + (v[1] * w[1])

And the runtime dropped from 7.09 seconds to 2.89 seconds, with the same results.


Find out whether string A can be shifted to get string B The task:

  Given two strings A and B, return whether or not A can be shifted some
  number of times to get B.

  For example, if A is abcde and B is cdeab, return true. If A is abc
  and B is acb, return false.

Solution 1:

const haveSameLength = (a, b) => a.length === b.length;
const isSame = (a, b) => a === b;
const isFullyShifted = a => a === 0;
const shiftStringBy = i => a => `${a.substring(i)}${a.substring(0, i)}`;

const isSameAfterShifting = (strA, strB, items) => {
  if (strA.length === 0 || strB.length ===0) { return false }
  if (!haveSameLength(strA, strB)) { return false }
  if (isSame(strA, strB)) { return true }
  if (isFullyShifted(items)) { return false }

  return isSameAfterShifting(strA, shiftStringBy(1)(strB), --items)
}

const str1 = 'abcde';
const str2 = 'cdeab';

console.log(isSameAfterShifting(str1, str2, str2.length));

Solution 2

const isSameAfterShifting2 = (strA, strB) => {
  if (strA.length === 0 || strB.length ===0) { return false }
  if (!haveSameLength(strA, strB)) { return false }
  const arrB = strB.split('');
  const firstLetterA = strA.substring(0, 1);

  let shiftIndex = arrB.indexOf(firstLetterA);
  if (shiftIndex === -1) { return false }
  while (shiftIndex < arrB.length) {
    const strBShifted = `${strB.substring(shiftIndex)}${strB.substring(0, shiftIndex)}`;
    if (strA === strBShifted) { return true }
    shiftIndex++;
  }
  return false;
}

console.log(isSameAfterShifting2('abc', 'acb'));

Which one is more readable and easier to understand for you?

You can check if the String is empty with !str instead of strA.length === 0,

console.log(''); // false

i think haveSameLength and isSame are extras, you can write srtA.length === strB.length and it would still be readable,

you can get the first letter with a simpler strA[0] instead of strA.substring(0, 1);

  Which one is more readable and easier to understand for you?

a loop is easier to read and understand than a recursive function,

But the hole approach seems like it can be simpler using a for loop, Array.some() , here's what i would suggest :

You can generate an array of combinations moving the letters one index at a time,
for a string abc you would have ['abc, bca', 'cba'], see if one of the resulting array entries euqals the second string :

const isSameAfterShifting = (str1, str2) => {
  // check if the strings are empty or has different lengths
  if (!str1 || !str2 || str1.length !== str2.length) return false;

  // check if the strings are the same
  if (str1 === str2) return true;

  // generate the array
  let combos = [];
  for (let i = 0; i < str1.length; i++) {
    let c = str1.slice(i) + str1.slice(0, i);
    combos.push(c);
  }

  // for a string 'abc'
  // combos = ['abc', bca', 'cab']

  // check if the array has one of its entries equal to the second string
  return combos.some(s => s === str2);
}

console.log( isSameAfterShifting('abc', 'cab') );
console.log( isSameAfterShifting('abc', 'cabaaa') );
console.log( isSameAfterShifting('abc', 'bac') );

you can replace the for loop with Array.from()

const isSameAfterShifting = (str1, str2) => {
  // check if the strings are empty or has different lengths
  if (!str1 || !str2 || str1.length !== str2.length) return false;

  // check if the strings are the same
  if (str1 === str2) return true;

  // generate the array
  let combos = Array.from({
    length: str1.length
  }, (_, i) => str1.slice(i) + str1.slice(0, i));

  // for a string 'abc'
  // combos = ['abc', bca', 'cab']

  // check if the array has one of its entries equal to the second string
  return combos.some(s => s === str2);
};

console.log(isSameAfterShifting("abc", "cab"));
console.log(isSameAfterShifting("abc", "cabaaa"));
console.log(isSameAfterShifting("abc", "bac"));


topological sorting to find the pre-requisite for courses Solving this problem:

There are a total of n courses you have to take, labeled from 0 to n - 1.
Some courses may have prerequisites, for example to take course 0 you have to first take course 1, which is expressed as a pair: [0,1]

Given the total number of courses and a list of prerequisite pairs, return the ordering of courses you should take to finish all courses.

There may be multiple correct orders, you just need to return one of them. If it is impossible to finish all courses, return an empty array.

I have used topological sorting to solve this problem. So, for every node I am calculating in-degree and picking the lowest node whose in-degree is zero. This will be the first node and who doesn't have any prerequisite. After that I am decrementing the child node in-degree and then repeating the same process until i am out of nodes.

import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.Queue;

public class Course_Schedule_II_210 {

    public static void main(String[] args) {
        int[][] arr = {};
        int totalCourse = 0;
        int[] totalCourseInOrder = findTotalCourseInOrder(totalCourse, arr);

        for (int i=0; i < totalCourseInOrder.length; i++) {
            System.out.println("The totalCourseInOrder element is : "+totalCourseInOrder[i]);
        }
    }

    private static int[] findTotalCourseInOrder(int totalCourse, int[][] arr) {

        HashMap<Integer, ArrayList<Integer>> map = new HashMap<Integer, ArrayList<Integer>>();
        int[] a1 = new int[totalCourse];
        int var = 0;

        for (int i=0; i < arr.length; i++) {
            int[] a = arr[i];
            int k = a[1];
            int v = a[0];
            if (!map.containsKey(k)) {
                map.put(k, new ArrayList<Integer>());
            }
            map.get(k).add(v);
            a1[v]++;
        }
        for (int j=0; j < a1.length; j++) {
            if (a1[j] == 0) {
                var = j;
                break;
            }
        }

        return findOrderInSeries(map, a1, var, totalCourse);
    }

    private static int[] findOrderInSeries(HashMap<Integer, ArrayList<Integer>> map, int[] a1, int var, int totalCourse) {

        int[] arr1 = new int[totalCourse];
        Queue<Integer> queue = new LinkedList<Integer>();

        int index = 0;
        for (int i = 0; i < a1.length; i++) {
            if (a1[i] == 0) {
                queue.add(i);
                arr1[index++] = i;
            }
        }
        while (!queue.isEmpty()) {
            int firstElement = queue.poll();
            if (map.get(firstElement) != null) {
                for (int ar : map.get(firstElement)) {
                    a1[ar]--;
                    if (a1[ar] == 0) {
                        queue.offer(ar);
                       arr1[index++] = ar;
                    }
                }
            }
        }
        for (int i = 0; i < a1.length; i++) {
            if (a1[i] != 0) {
                return new int[0];
            }
        }
        return arr1;
    }

}

Thank you for sharing your code.

Here are my thought on it:

OOP

Your code is a procedural approach to the problem.

There is nothing wrong with procedural approaches in general, but Java is an object oriented (OO) programming language and if you want to become a good Java programmer then you should start solving problems in an OO way.

If you don't care about OOP you should look for a different programming language that better supports procedural or functional approaches.

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Naming Conventions

Please read (and follow) the
Java Naming Conventions

Your class name violates the Java naming conventions. Maybe you got the name provided by your teacher. Then you have to use it of cause. But keep in mind that you should not use underscores in your own identifier names (except constants).

avoid single character names / abbreviations

Since the number of characters is quite limited in most languages you will soon run out of names. This means that you either have to choose another character which is not so obviously connected to the purpose of the variable. And/or you have to "reuse" variable names in different contexts. Both makes your code hard to read and understand for other persons. (keep in mind that you are  that other person yourself if you look at your code in a few month!) The same is true for (uncommon) abbreviations. You might find them being obvious today, while you're actively dealing with the problem, but You might have to "relearn" them if you worked on something else for a while.

On the other hand in Java the length of identifier names names is virtually unlimited. There is no penalty in any way for long identifier names. So don't be stingy with letters when choosing names.

As usual there are some exceptions for this suggestion.
E.g. the running index of loops is usually held in a single letter named variable like i in

for(int i =0; i<SOME_CONSTANT; i++)

But even there you should use more problem oriented names to support your readers, especially in nested loops like this:

for(int row =0; row<ENTRY_COUNT; row++)
   for(int column =0; column<ENTRY_FIELD_COUNT; column++)

Choose you names from the problem domain, not from the technical solution.

You have variable names like map, k (as for key) and v (as for value). Better names for them might be like

    int course = a[1];
    int prerequest = a[0];
    if (!prerequestsForCourse.containsKey(course)) {
        prerequestsForCourse.put(course, new ArrayList<Integer>());
    }
    prerequestsForCourse.get(course).add(prerequest);

Magic numbers

your code has some magic numbers. This are literal values with a special meaning like here:

int totalCourse = 0;
//...
int k = a[1];
int v = a[0];

You should introduce constants with meaningful names:

int totalCourse = INITIAL_COUNT_IS_ZERO;
//...
int k = a[INDEX_OF_COURSE];
int v = a[INDEX_OF_PREREQUEST];

Know your language / read the API

You are using an legacy pattern to ensure the existence of the  the course in the map to the list of its prerequests. You may have found that or have been told to use it. But you should always look at the API of the class or interface you use if there is a better solution. You would have found this
https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-
which would simplify your code to this:

prerequestsForCourse.computeIfAbsent(course,()->new ArrayList()).add(prerequest);

Single Layer Of Abstraction

This principle states that a method (or class) should either do simple/primitive calculations or (exclusive) call other methods (on other objects).

The last line in your method findTotalCourseInOrder() is the call to findOrderInSeries() which let me assume that you already know how to extract a bunch of lines to a separate method. You should do this with the the for loop above too:

private static int[] findTotalCourseInOrder(int totalCourse, int[][] arr) {
    HashMap<Integer, ArrayList<Integer>> map = new HashMap<Integer, ArrayList<Integer>>();
    int[] a1 = new int[totalCourse];
    int var = mapPrerequestsToCourses(arr, map, a1);
    return findOrderInSeries(map, a1, var, totalCourse);
}

private static int mapPrerequestsToCourses(int[][] arr, HashMap<Integer, ArrayList<Integer>> map, int[] a1) {
    int var = 0;

    for (int i = 0; i < arr.length; i++) {
        int[] a = arr[i];
        int k = a[1];
        int v = a[0];
        if (!map.containsKey(k)) {
            map.put(k, new ArrayList<Integer>());
        }
        map.get(k).add(v);
        a1[v]++;
    }
    for (int j = 0; j < a1.length; j++) {
        if (a1[j] == 0) {
            var = j;
            break;
        }
    }
    return var;
}

avoid break

break (as well as continue) are javas substitute for goto. Using them leads to code that is both, hard to read and hard to maintain/refactor.

You should not use break outside of a switch block.

An alternative implementation without the break statement could be based on the previous suggestion. In that modification the value returned is the result of the for loop. So you could simply exchange  break; by return j;

define variables as late as possible

In all you methods you define all variables right at the top of the method.

In Java this is not needed (as it is in C).

The problem with that is that it confuses your IDEs automated refactoring feature.

E.g.:  Your variable var is only used in the last for loop in findTotalCourseInOrder().
If you want to apply the extract method refactoring to this last for loop you get the variable var as a parameter to the new method.

    var = extracted(a1, var);
    return findOrderInSeries(map, a1, var, totalCourse);
}

private static int extracted(int[] a1, int var) {
    for (int j = 0; j < a1.length; j++) {
        if (a1[j] == 0) {
            var = j;
            break;
        }
    }
    return var;
}

If you'd move the declaration of variable var right before the loop you could include this line in the selection and the new method would not have this extra parameter.
Your code:

    int var = 0;
    for (int j = 0; j < a1.length; j++) {
        if (a1[j] == 0) {
            var = j;
            break;
        }
    }
    return findOrderInSeries(map, a1, var, totalCourse);
}

would change to:

    int var = extracted(a1);
    return findOrderInSeries(map, a1, var, totalCourse);
}

private static int extracted(int[] a1) {
    int var = 0;
    for (int j = 0; j < a1.length; j++) {
        if (a1[j] == 0) {
            var = j;
            break;
        }
    }
    return var;
}


Shuffle a deck of cards, using the overhand shuffle method I've started learning Clojure, after many years of Java. As one of a series of exercises, I'm writing a simple card game, and the overhand-shuffle function is shown below.

My main concerns are:

Most of my code appears inside the let, and to me it looks more procedural than functional, is this the best way to do this? Please bear in mind that the reasons I did it this way are:

I wanted to avoid having the body of the function as one big, monolithic block of dense, unreadable code
Binding lots of symbols like this chops the code into manageable pieces, making it easier for me to read and understand intermediate steps
Debugging is also easier, as I can examine what's happening at each step

In my reduce, I'm passing back two values on each iteration (stored in the map result). These are :cards (which contains cards not yet allocated) and :new-cards (which builds up the new version of shuffled cards). However, I'm only interested in the new, shuffles cards being passed back as the result of the reduce, not the intermediate results that reduce needs at each step. Is there a cleaner way to do this?

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (let [number-of-cards   (count cards)
        number-of-slices  (/ (* number-of-cards 7) 52)
        max-slice-size    (int (/ (* number-of-cards 0.6) number-of-slices))

        ; Randomly determine the size of the little slices we will take
        slice-sizes
          (for [_i (range number-of-slices)]
            (inc (rand-int max-slice-size))
            )

        slice-total       (apply + slice-sizes)
        top-card-count    (- number-of-cards slice-total)

        ; Take the top cards, and make slices out of the remaining cards
        top-cards         (take top-card-count cards)
        slice-cards       (drop top-card-count cards)

        ; Chop up the slices, and return with them in reverse order
        slices
          (reduce
            (fn [result slice-size]
              (let [cards     (:cards result)
                    new-cards (:new-cards result)

                    ; Take a slice, being the first n of the remaining cards
                    slice     (take slice-size cards)
                    ]
                ; Return with the remaining cards and the growing list of new cards
                {:cards     (drop slice-size cards)
                 :new-cards (concat slice new-cards)}
                )
              )
            ; initial results we will start the reduce with
            {:cards slice-cards :new-cards nil}

            ; iterate over the list of slice sizes
            slice-sizes)
        ]
    ; We have what we need, so return with the slices on top of the remaining cards
    (concat
      ; The slices that came from the bottom of the cards
      (:new-cards slices)

      ; Add what used to be the top cards, after the slices
      top-cards
      )
    )
  )

(defn test-shuffle
  "Simple invocation of shuffle, to show result"
  []
  (let [simple-cards (range 1 53)
        shuffle-times 10]
    (loop [count shuffle-times
           cards simple-cards]
      (if (< count 0)
        cards
        (recur (dec count) (overhand-shuffle cards))
        )
      )
    )
  )

A quick disclaimer: I am not really a clojure developer so take any of my recommendations with a pinch of salt.

Regarding your giant let:  I don't think that's necessarily a bad thing to do, but in some cases it might be worth splitting some of it off into separate functions.  For example max-slice-size and number-of-slices are only really needed to generate slice-sizes, so it may be worth splitting them off into a make-slice-sizes function or something.

Most of the reasons you listed for having things in a giant let are actually really good reasons to use smaller functions:

You've got a selection of smaller functions rather than the big dense mono-function you were trying to avoid.
Your individual functions are your manageable pieces, rather than each binding of the let.  You can call them from a repl, or write tests of just those individual functions to make sure they are behaving as expected.
Debugging is also still possible, but with the benefit that it's easier to use a repl & write tests.

A few points about your reduce:

You can simplify it a bit by returning a vector instead of a map and using argument destructuring instead of getting the values out of the map.
When you want to do a take & a drop using the same numbers it's usually easier to use split-at.
Passing back 2 things from a reduce doesn't seem wrong.  However I would consider just using your reduce to build up a list of slices that you can manipulate afterwards rather than using concat to actually do the shuffling within the reduce.  The reductions function can help with this - it is like reduce, but it returns the intermediate steps that you can manipulate later.
Currently you are building up a list of random slice sizes, and then writing some more code to handle the remainder of the cards (top-cards in your code).  I think you could simplify your code a fair bit by just treating this as another slice and adding it in to your slice-sizes sequence.

I've had a go at implementing my suggestions, and ended up with this:

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (let [slice-sizes        (make-slice-sizes (count cards))
        ; The first value returned by reductions is the initial value, so we
        ; drop it here.
        [_ & slices]       (map first
                             (reductions (fn [[_ remaining-cards] slice-size]
                                           (split-at slice-size remaining-cards))
                                         [nil cards]
                                         slice-sizes))]
    (apply concat (reverse slices))))

(defn- make-slice-sizes
  "Randomly determine the size of the little slices we will take"
  [number-of-cards]
  (let [number-of-slices  (/ (* number-of-cards 7) 52)
        max-slice-size    (int (/ (* number-of-cards 0.6) number-of-slices))
        slice-sizes       (map inc (repeatedly
                                    number-of-slices
                                    #(rand-int max-slice-size)))
        remaining-cards   (- number-of-cards (apply + slice-sizes))]
    ;; We need to make sure all our cards get "shuffled", so add a slice of the
    ;; remaining cards on the end of our list of slices
    (concat slice-sizes [remaining-cards])))

Please let me know if I've explained any of this badly, and I'll try and improve it.

Edit: You can actually take this a bit further and split out your reduction into a function as well:

(defn build-slices
  "Builds a sequence of slices from cards.
   The sizes of the slices are determined using slice-sizes"
  [cards slice-sizes]
  ;; Perform a reduction where the first element of each step is the slice,
  ;; and the second element is the remaining cards.  Use a let binding to get
  ;; rid of the first reduction, because it is just the initial value we
  ;; passed in.
  (let [[_ & slices]  (map first
                          (reductions (fn [[_ remaining-cards] slice-size]
                                        (split-at slice-size remaining-cards))
                                      [nil cards]
                                      slice-sizes))]
    slices))

Which allows you to simplify your overhand-shuffle function further:

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (let [slice-sizes   (make-slice-sizes (count cards))
        slices        (build-slices cards slice-sizes)]
    (apply concat (reverse slices))))

Since both these functions are now just a series of steps each building on the last you could rewrite using one of the threading macros (-> or ->>).  These let you list a series of forms and automatically insert the result of the last form into the next form, as either the first (for ->) or the last (for ->>) parameter to each of them.  This allows you to make your function a series of "steps" that transform your input data, a bit like your original let bindings, but without the intermediate variables:

(defn build-slices
  "Builds a sequence of slices from cards.
   The sizes of the slices are determined using slice-sizes"
  [cards slice-sizes]
  (->> slice-sizes
       ;; Perform a reduction where the first element of each step is the slice,
       ;; and the second element is the remaining cards.
       (reductions (fn [[_ remaining-cards] slice-size]
                     (split-at slice-size remaining-cards))
                   [nil cards])
       ;; Drop the first element, as it is just our initial [nil, cards] value.
       (drop 1)
       ;; Extract each of the slices.
       (map first)))

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (->> cards
       (count)
       (make-slice-sizes)
       (build-slices cards)
       (reverse)
       (apply concat)))

A bonus edit: the loop in your test-shuffle function is very procedural.  Could be better with iterate:

(defn test-shuffle
  "Simple invocation of shuffle, to show result"
  []
  (let [simple-cards (range 1 53)
        shuffle-times 10]
    (nth (iterate overhand-shuffle simple-cards) shuffle-times)))


Converting from polar coordinates to rectangular coordinates This is performance critical. I measured and determined that using the sqrt is faster then using the cos method.

I am aware that this code only works for some points, so that is not an issue.

Point is System.Drawing.Point. _offset is also of type Point.

I assumed, and the profiler seemed to confirm, that the try/catch will not slow down the code unless an exception occurs. Please correct me if that is wrong.

/// <summary>
/// Convert from polar coordinates to rectangular coordinates.
/// Only works for points to the left of the origin.
/// </summary>
/// <param name="radius">The radius of the point in pixels.</param>
/// <param name="theta">The angle of the point in radians.</param>
/// <returns>The point in rectangular coordinates.</returns>
internal Point PolarToRectangular(
        double radius,
        double theta)
{
    try
    {
        double sin = Math.Sin(theta);

        // This is faster then:
        // double cos = Math.Cos(theta);
        double cos = -Math.Sqrt(1 - (sin * sin));

        Int32 x = _offset.X + (Int32)Math.Round(radius * cos);
        Int32 y = _offset.Y + (Int32)Math.Round(radius * sin);

        return new Point(x, y);
    }
    catch (OverflowException ex)
    {
        ex.Data.Add("Screen polar Radius", radius);
        ex.Data.Add("Screen polar Theta", theta);
        throw;
    }
}

I would consider using a lookup table to find your sines. How precise an angle are you calculating for? This answer on stackoverflow has some interesting information on how to construct one and the benchmarks for using it to two decimal points. If you are using the full precision of a double, then your current method will be faster, but if you have a fixed precision, a lookup table will be benificial.

As long as the exception IS exceptional (and it looks to me like it is) it will add very little overhead to your run times. See: https://stackoverflow.com/q/52312/487663.


Determining the difference between odd and even numbers I have code that converts each character of a String to an int and returns the difference between odd and even numbers. Can this code be simplified?

int compareSumOfDigits(String N) {
int e=0,o=0;
for (int i =0;i<N.length();i++){
    int t = Character.getNumericValue(N.charAt(i));
    if(t%2==0)
    e+=t;
else
    o+=t;

}
      return  o-e ;
}

Combining some of the existing answers, you can do

s.chars()                          // get the char stream
  .map(Character::getNumericValue) // convert to ints
  .map(n -> n%2==0 ? -n : n)       // negate the even ones
  .sum()                           // sum it all up

This will give you the sum of the odds and the negative evens, which is the same as the sum of the odds minus the sum of the evens.

edit

In response to @kai, for absolute max readability, I'd probably do (pseudocode)

List<int> ints = s.chars().map(Character::getNumericValue)
Map<Boolean, List<int>> intsEven = ints.partitioningBy(n -> n%2==0)
return intsEven.get(false).sum() - intsEven.get(true).sum()

or with isEven from the other answers and not defined here

List<int> ints = s.chars().map(Character::getNumericValue)
List<int> evens = ints.filter(isEven)
List<int> odds = ints.filter(not(isEven))
return odds.sum() - evens.sum()


Check disk space against a threshold The below script is one I made to check the disk space on mounted partitions under Debian Wheezy. I tried adding a -d switch to force printing out the used data, but its main purpose is to run as a cron job, and email the administrator if any disks are below a certain threshold.

What I'm looking for is to make sure it's POSIX compliant (in case it needs to be called by another script/application), and that it's rock solid. It works on my servers as is, with and without the -d. It requires that if an argument is given, it's a mount point. I am trying to get the proper coding practices done properly, as well.

#!/bin/sh

MOUNTPOINTS="/ /var /home" #The mount points to always check; the user can
                           #specify more via the command line, or changing this.
THRESHOLD=85    #The amount of disk space used before printing the warning.
DEBUG=false     #If TRUE when running (-d switch) lists all mount points, even if
                #the threshold has not been met.

while getopts ":dh" opt; do
        case $opt in
                d)
                        DEBUG=true
                        ;;
                h)
                        echo USAGE: $0 \[-d\] \[\/mount\/point\/1 ...\]
                        exit 0
                        ;;
                \?)
                        echo Incorrect syntax
                        ;;
        esac
        shift $((OPTIND - 1))
done

if [ "$#" -gt 0 ]; then
        for var in $@; do
                MOUNTPOINTS="${MOUNTPOINTS} ${var} "
        done
fi

for MOUNT in ${MOUNTPOINTS}; do
  CURRENT=$(df ${MOUNT} | grep / | awk '{ print $5}' | sed 's/%//g')
  if ($DEBUG); then
    printf "%20s\t%s\n" ${MOUNT} ${CURRENT}%
  fi
  if [ "${CURRENT}" -gt "${THRESHOLD}" ] ; then
      echo "Your ${MOUNT} partition\'s remaining space is critically low. Used: ${CURRENT}%"
    fi

done

Using a string for MOUNTPOINTS means you can't have any mount points with spaces of glob characters in the names. I realize you likely can't use an array if you want real portability but that's a severe limitation that you might want to check for.

I don't know what you can do (without using an array) that will allow directories like that to work but you can check the input arguments for compatibility and disallow any that don't fit those limitations.

As I said on some other question (of yours I believe) that shift in the while loop is incorrect. You are shifting off (an increasing number of arguments) during the getopts processing. You want to do that shift once and after getopts is done.

Try this and see what I mean:

$ cat test-getopts.sh
#!/bin/sh

while getopts ":dh" opt; do
    case $opt in
        d) echo DEBUG=true;;
        h) echo HELP;;
        \?) echo error;;
    esac
    printf 'Shifting %d\n' $((OPTIND - 1))
    shift $((OPTIND - 1))
done
$ /bin/sh go.sh -d -h -d -h -d -h -d -h -d -h -d -h
DEBUG=true
Shifting 1
DEBUG=true
Shifting 2
HELP
Shifting 3
HELP
Shifting 4

There is almost never a reason to pipe grep to awk or sed to awk or awk to sed. awk can  almost certainly do everything you need.

So the grep | awk | sed part of $(df ${MOUNT} | grep / | awk '{ print $5}' | sed 's/%//g') can be replaced by a single awk call:

`$(df "${MOUNT}" | awk '/\/{gsub(/%/, ""); print}')`

You'll also notice I quoted "${MOUNT}" there. In general you always want to quote shell variable expansions. In this case it can't actually matter since anything these quotes would protect will already have been expanded/word-split by the for MOUNT in ${MOUNTPOINTS} loop but it is the right thing to do anyway.

Also related to df if you are going to try to field-parse the output you want to use the -P/--portability option. It keeps df (at least from coreutils) from wrapping long lines to get columns to line up (and breaking field counts).

The syntax of if tests in the shell does not require wrapping parentheses. In fact, adding them spawns a sub-shell. So if ($DEBUG); then is running the value of $DEBUG as a command in a sub-shell. If you were to have set DEBUG=debug instead of DEBUG=true you'd have gotten an error from that because debug isn't a valid command. Drop them and use a non-empty or value checking test instead. Either if [ -n "$DEBUG" ]; or if [ "$DEBUG" = DEBUG ];.

You don't need to quote single quotes in a double quoted string so:

echo "Your ${MOUNT} partition\'s remaining space is critically low. Used: ${CURRENT}%"

should be echoing partition\'s out which probably isn't what you want.

for var in $@; do can be simplified to for var; do if you wanted and if you don't then you should use for var in "$@"; do though again given that spaces/glob characters are going to be a problem later this isn't as useful as it might be (though it does let you test for them correctly and avoid them as discussed above).

Also, if you haven't already, you should run your code through shellcheck.


Determining numeric palindromes I am working on bettering my C# skills, and recently wrote this program to solve a projecteuler.net problem:

  A palindromic number reads the same both ways. The largest palindrome
  made from the product of two 2-digit numbers is 9009 = 91 * 99. Find
  the largest palindrome made from the product of two 3-digit numbers.

The code works and produced the correct answer, but I'm looking to avoid bad code and use best practices. Any critique of changes to make this more efficient would be appreciated! (Also, if there are any WTF's in there, please point them out!)

using System;
using System.Collections.Generic;
using System.Text;

namespace NumericPalindrome {
    class Program
    {
        static void Main(string[] args)
        {
            long result = 0;

            for (int i = 100; i < 1000; i++)
            {
                for (int k = 100; k < 1000; k++)
                {
                    if (IsPalindrome(i * k))
                        if ((i * k) > result)
                            result = i * k;
                }
            }

            Console.WriteLine(result);
        }

        static bool IsPalindrome(long testNumber)
        {
            List<int> list = new List<int>();

            //Break testNumber into an integer list of its digits
            while (testNumber >= 1)
            {
                list.Add((int)(testNumber % 10));
                testNumber /= 10;
            }

            List<int> reverseList = new List<int>(list);
            reverseList.Reverse();

            bool palindrome = true;

            for (int i = 0; i < list.Count; i++)
            {
                if (list[i] != reverseList[i])
                    palindrome = false;
            }

            return palindrome;
        }
    }
}

I do not do C# but I can offer another way of approaching the problem:

As our range of numbers is \$[100\cdot100, 999\cdot999] = [10000, 998001]\$, assume a palindromic number on the form \$abccba\$.

The largest \$abccba < 998001\$ is \$k = abccba = 997799\$. We need to determine if \$k\$ can be written as the product of two 3-digit numbers. Using a prime table (or generate one with a sieve at program start) see if any prime number \$p\$ where \$\min\left(\frac{k}{100}, 999\right) \lt p\le k\$ evenly divides \$k\$.

If you can find such a prime then we know that \$\frac{k}{p} \lt 100 \$  and any remaining factors are too small or that \$p \gt 999\$ and we have a factor that is too large. So we can discard \$k\$ and decrement \$c\$ by one to form the next smaller palindrome, eg. \$k=996699\$, and keep trying smaller palindromes.

If you reach \$p\lt \min\left(\frac{k}{100}, 999\right) \$ without finding any prime that evenly divides \$k\$ we need to convince ourselves that the remaining factors can be multiplied into two factors so that both factors are three digits.

There are a few ways to convince ourselves of the above. One way is trial division by simply trying all divisors \$d\$ such that \$100\le d \le \max\left(\frac{k}{100}, 999\right)\$ and checking what the other factor is.

Another more elegant way is to use the fact that we know that there is no prime factor larger than \$p\le\min\left(\frac{k}{100}, 999\right)\$ and find the largest prime factor \$p_{max}\le\min\left(\frac{k}{100}, 999\right)\$ and let \$r_i = \frac{k}{p_{max}}\$.

Let the prime factors of \$r_i\$ be \$p_i\$ and let \$Q=\prod_{\forall i:p_i>\frac{999}{p_{max}}}p_i\$. At this point you have the largest prime factor of \$k\$ as \$p_{max}\$ and all prime factors of \$r_i=\frac{k}{p_{max}}\$ that are too big to be together with \$p_{max}\$ multiplied together as \$Q\$.

Now, factor \$\frac{r_i}{Q}\$ into primes \$P_k\$. Each of the \$P_k\$ primes has to be multiplied into either \$Q\$ or \$p_{max}\$. Iteratively try all combinations (and exit early for impossible branches) until you find one that is satisfactory or retry next smallest palindrome if no combination was found.

Note: This may well be slower than trial multiplication but it's another way to approach the problem and it may give birth to other ideas.


Top K frequent elements I am working on an interview question where I need to return the k most frequent elements given a non-empty array of integers.

Input: nums = [1,1,1,2,2,3], k = 2
Output: [1,2]

Here is my code:

  public static void main(String[] args) {
    System.out.println(topKFrequent(new int[] {1, 1, 1, 2, 2, 3, 3}, 1));
  }

  private static List<Integer> topKFrequent(int[] nums, int k) {
    // freq map
    Map<Integer, Integer> freq = new HashMap<Integer, Integer>();
    for (int n : nums) {
      freq.put(n, freq.getOrDefault(n, 0) + 1);
    }
    // bucket sort on freq
    List<Integer>[] bucket = new ArrayList[nums.length + 1];
    for (int i = 0; i < bucket.length; i++)
      bucket[i] = new ArrayList<>();
    for (int key : freq.keySet()) {
      bucket[freq.get(key)].add(key);
    }
    // gather result
    List<Integer> res = new ArrayList<>();
    for (int i = bucket.length - 1; i >= 0; i--) {
      res.addAll(bucket[i]);
      if (res.size() >= k)
        break;
    }
    return res;
  }

I wanted to check whether there is any improvement/optimization can be made in this code?

Instead of looping over the freq.keySet, I recommend using freq.forEach((k, v) -> bucket[v].add(k)). This avoids the map lookups. In your current code, for each key you do a search in the map, which takes time. By using forEach, this additional search is avoided.

Do you actually have any performance requirements? If not, I'd say the code is perfect. It is easy to read and understand, and well documented.

Your code might return more than the k elements that are requested, because of the addAll. This should be discussed with the interviewer whether it is ok.

Too bad that Java doesn't have a built-in Histogram class. Things would have been much simpler that way:

new Histogram(nums).stream()
    .limit(k)
    .forEach(System.out::println);


Find minimum and maximum coordinates for a set of points I wrote the Python3 code below. This is actually a stripped down version of a bigger project, but in essence it boils down to this code. There are two classes: Point, which is a point in 3D space and PointCloud, which is a sequence (list) of points.

In the PointCloud class, I have a method to calculate the minimum and maximum values for each axis: minmax_coords, which returns a dict (keys: the axes 'x', 'y', 'z') of a dict (keys: 'min' and 'max'). So you can do: somepointcloud.minmax_coords()['x']['min'] for the minimal x-axis value.

As it is, the code doesn't feel very pythonic and isn't really self-explanatory.

My question is: please review the code of the minmax_coord method and give me some suggestions how to refactor the code to make it more pythonic and more self explanatory.

Any suggestions to improve the code are welcome.

Repl link to the code.

class Point:
    """ Point with coordinate """
    def __init__(self, coordinate=None):
        """ Constructor """
        self.coordinate = coordinate

class PointCloud:
    """ A cloud (list) of Points """
    def __init__(self, point_list=None):
        """ Constructor """
        self.points = point_list

    def minmax_coords(self):
        """ Return minimum and mainum coordinate for each axis """
        if not self.points:
            raise IndexError("PointCloud has no Points.")

        result = {'x': {}, 'y': {}, 'z': {}}
        for i, axis in zip(range(3), ['x', 'y', 'z']):
            result[axis]['min'] = self.points[0].coordinate[i]
            result[axis]['max'] = self.points[0].coordinate[i]

        for point in self.points:
            for i, axis in zip(range(3), ['x', 'y', 'z']):
                result[axis]['min'] = min(result[axis]['min'],
                                          point.coordinate[i])
                result[axis]['max'] = max(result[axis]['max'],
                                          point.coordinate[i])

        return result

if __name__ == "__main__":
    P = PointCloud([Point((1, 2, 3)), Point((5, 4, 3))])
    print(f"Minimum and maximum coordinates: {P.minmax_coords()}")

As @200_success mentions, these classes seem a bit sparse, and you shouldn't bother with classes for them in the first place.

For instance, Point could be nicely replaced with a namedtuple.

>>> from collections import namedtuple
>>> Point = namedtuple('Point', ['x', 'y', 'z'], defaults=[0,0,0])
>>> p1 = Point(1,4,3)
>>> p2 = Point(5,2,3)

The components of Point can now easily be referred to by name (eg, p1.x or p2.z) and can still be referred to by index (eg, p1[0] or p2[2]).

Similarly, you don't need a whole class just for a minmax_coords function:

>>> def minmax_coords(*pnts):
...    mn = Point( *(min(p[i] for p in pnts) for i in range(3)) )
...    mx = Point( *(max(p[i] for p in pnts) for i in range(3)) )
...    return mn, mx
...
>>> minmax_coords(p1, p2)
(Point(x=1, y=2, z=3), Point(x=5, y=4, z=3))
>>>

Note: As opposed to a dictionary of dictionaries for the result, I've changed the return into two points: a point with the minimum coordinate on each axis, and a point with the maximum coordinate on each axis.

Demystifying the minmax_coords implementation:

min(p[i] for p in pnts) computes the minimum value of the i'th axis over all of the points.
_____ for i in range(3) is a generator expression which executes that minimum computation for each axis i=0, i=1 and i=2.
Point( *(____) ) immediately evaluates the generator expression and uses the values as the 0th, 1st, and 2nd arguments of the Point() constructor.


Find all the paths of tree that add to a input value This code is attributed to Stack Overflow itself here. The path which adds to the sum, which need not be starting from the root, not be ending at the leaf, i.e. a path can be between root and a leaf.

I'm looking for code review, optimizations and best practices. I'm also looking for a solution better than \$O(n^2)\$ time complexity, if such exists. I'm also verifying space complexity to be \$O(n)\$, where \$n\$ in both the cases is the number of treenodes.

Also, the code specifically demands me to print, making unit testing not possible. Are there any pointers on how such code could be tested?

public class AllPathsAddingSum {

    private TreeNode root;

    public AllPathsAddingSum(List<Integer> items) {
        create (items);
    };

    /**
     * Constructs a binary tree in order of elements in an array.
     * After the number of nodes in the level have maxed, the next
     * element in the array would be a child of leftmost node.
     */
    private void create(List<Integer> items) {
        if (items.size() == 0) {
            throw new IllegalArgumentException("There should atleast be single item in the tree");
        }

        root = new TreeNode(items.get(0));

        final Queue<TreeNode> queue = new LinkedList<TreeNode>();
        queue.add(root);

        final int half = items.size() / 2;

        for (int i = 0; i < half; i++) {
            if (items.get(i) != null) {
                final TreeNode current = queue.poll();
                final int left = 2 * i + 1;
                final int right = 2 * i + 2;

                if (items.get(left) != null) {
                    current.left = new TreeNode(items.get(left));
                    queue.add(current.left);
                }
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode(items.get(right));
                    queue.add(current.right);
                }
            }
        }
    }

    private static class TreeNode {
        private TreeNode left;
        private int item;
        private TreeNode right;

        TreeNode(int item) {
            this.item = item;
        }
    }

    /**
     * Prints all the paths that add to the input value.
     * @param value the value, that all the nodes of a path should add upto.
     */
    public void printAllSumPaths(int value) {
        traverse(root, value);
    }

    private void traverse(TreeNode node, int value) {
        if (node == null) return;

        computePaths(node, 0, value, new ArrayList<Integer>());

        traverse(node.left, value);
        traverse(node.right, value);
    }

    private void computePaths(TreeNode node, int sum, int value, List<Integer> path) {
        if (node == null) return;

        sum = sum + node.item;
        path.add(node.item);

        if (sum == value) {
            for (Integer i : path) {
                System.out.print(i + " ");
            }
            System.out.println();
        }

        computePaths(node.left, sum, value, path);
        computePaths(node.right, sum, value, path);

        path.remove(path.size() - 1);
    }

    public static void main(String[] args) {
        AllPathsAddingSum sumPaths = new AllPathsAddingSum(Arrays.asList(1, 2, 3, 4, 5, 6, 7));
        sumPaths.printAllSumPaths(4);

        sumPaths = new AllPathsAddingSum(Arrays.asList(1, 0, 0, -1));
        sumPaths.printAllSumPaths(0);
    }
}

public AllPathsAddingSum(List<Integer> items) {
    create (items);
};

Unneeded space, unneeded semicolon. Consider autoformatting your code.

for (Integer i : path) {
    System.out.print(i + " ");
}
System.out.println();

This section in computePaths is doing something different than the function description. It doesn't calculate, it prints.

Move it to a separate method. Then give the class a PrintStream to use. This PrintStream would be System.out, or a dummy PrintStream that just copies everything to an internal buffer.

Then you can unittest your code.


Reading a yes/no answer from std::cin Based on this question, I wrote my own implementation of the yesno function I suggested. The function reads an answer until it is either "y" or "yes" or "n" or "no" (case-insensitive).

#include <algorithm>
#include <cctype>
#include <cstdlib>
#include <iostream>
#include <string>

bool yesno(const std::string &prompt)
{
    while (true) {
        std::cout << prompt << " [y/n] ";

        std::string line;
        if (!std::getline(std::cin, line)) {
            std::cerr << "\n";
            std::cerr << "error: unexpected end of file\n";
            std::exit(EXIT_FAILURE);
        }

        std::transform(line.begin(), line.end(), line.begin(),
                       [](unsigned char x){return std::tolower(x);});

        if (line == "y" || line == "yes") {
            return true;
        }
        if (line == "n" || line == "no") {
            return false;
        }
    }
}

int main()
{
    bool include_digits = yesno("Should the password include digits?");
    if (include_digits) {
        std::cout << "le1mein\n";
    } else {
        std::cout << "letmein\n";
    }
}

I took care to:

Include all necessary headers.
Call the std::tolower function with an unsigned char as argument.
Use std::getline instead of the hard-to-control >> operator.
Catch all errors (except for writing to std::cout).

Anything that I missed?

Good job in general, mad props for proper use of cout vs cerr.

I don't see any "errors" per se, but I do see a few things I personally would have done a bit differently.

Opinion: Not a big fan of std::exit() in anything but main()

A function is supposed to be decontextualized, and having a "will crash the whole program if it fails" as part of the function's contract is just too aggressive in my opinion.

Personally, I'd rather just throw an exception instead. It accomplishes the same thing, and it lets users of the function choose how to handle failures.

Opinion: maybe have a separate prompt for failure?

I would personally explain to the user why he is being asked the same question twice when the input fails to match either criteria.

Opinion: std::transform is overkill here

This is a personal bias, as I find in-place usage of std::transform uncomfortable at best, since a range-based for does as good a job in a much more legible manner.

Specifically: Having to read the third parameter to understand that it's an inplace transform is just unnecessary cognitive load. I find the following easier to parse:

for(auto& x : line) {
  x = std::tolower(static_cast<unsigned char>(x));
}


Span decimal point precision In a JSP page I have a number of Span elements. Each of this span element will be floating point numbers.

All these span elements has class "format-decimal-precision":

${info.flaot_val}

I would like to display all this floating point numbers with 3 decimal point precision.

Example: 123.12867 should be displayed as 123.123

var floatFields = $('.format-decimal-precision');
    var iter = floatFields.length;
    var mDecDisplay = 3;

    while(iter--) {
        floatFields[iter].innerHTML=roundNumber($.trim(floatFields[iter].innerHTML),mDecDisplay);
    }

    function roundNumber(num, places) {
        return Math.round(num * Math.pow(10, places)) / Math.pow(10, places);
    }

Though the code is working fine, I would like to know a better program to achieve the same behavior.

Styling and readability

Your indentation is inconsistent. This can lead to developers wrongly assuming code belongs to a different block. I would recommend fixing this for all code.

Your use of whitespace is inconsistent. You sometimes use a space after a comma, you sometimes do not. You sometimes use spaces around operators, you sometimes do not.

Re-inventing the wheel

Your function roundNumber is somewhat re-inventing the wheel. You are basically recreating the toFixed function with a cast back to a number.

function roundNumber(num, places) {
  return Math.round(num * Math.pow(10, places)) / Math.pow(10, places);
}

function roundNumberFixed(num, places) {
  return Number(num).toFixed(places);
}

function roundNumberFixedAsNumber(num, places) {
  return Number(Number( num ).toFixed(places));
}

function test(num, places) {
  console.log("== Testing with", num, "and", places, "==");
  console.log(roundNumber(num, places));
  console.log(roundNumberFixed(num, places) );
  console.log(roundNumberFixedAsNumber(num, places));
}

var testvalues = [["1.234567", 3],
                  [" 1      ", 2],
                  ["123512.2", 2],
                  ["-55.2121", 5],
                  ["13.223  ", 1]];
console.log("=== Starting tests ===");
for(i in testvalues) {
  test(testvalues[i][0], testvalues[i][1]);
}

You seem to be using jQuery. You can use .each (docs) to loop over all elements in an jQuery object. Similarly, you can use .html (docs) to change the innerHTML of an element.

Termination of loop on referenceError

You use a while loop without a way of terminating it. It actually terminates, because you try to modify the innerHTML of undefined. That's... silly.