Object-oriented widget for reuse I have been using a pattern similar to this basic structure below. Can anyone please let me know if this is a correct way to code an object-oriented widget for reuse?  I am looking to make code than can also be extended easily and modular.

$(function(){
// wrap in jquery .fn wrapper so that may be called on a
function $lider(autoplay, velocity, controls){
  var $lider = [
    // props n methods, accessible through data-slider-* attributes
    {
      settings : {
        envokeBecause : $('[data-widget~="slider"]'),
        autoplay : autoplay,
        speed : velocity,
        showControls : controls,
        slideSize : '100%',// 25% will show 4 slides if the parent container is 100% width and so on
      },
      bindings : {
        slideRail : $('[data-function~="slide-rail"]'),
        slide : $('[data-function~="slide"]'),
        nextButton : $('[data-function~="next"]'),
        prevButton : $('[data-function~="prev"]'),
        playButton : $('[data-function~="play"]'),
        pauseButton : $('[data-function~="pause"]'),
        stopButton : $('[data-function~="stop"]')
        // attach this functionality to the DOM
      },
      methods : {
        slideNext :  function(){ slideRail.animate({left: '-=100%'}, velocity)   },
        slidePrev : function(){ slideRail.animate({left: '+=100%'}, velocity)  },
        slideRestart : function(){ slideRail.animate({left: '0%'}, velocity)  },
        slideStart :  function(){ window.$liderTimer = setInterval(slideNext, velocity) },
        slidesCount : function(){ slideRail.children().size()
        }
      }
    }
  ]
  $.each($lider, function(){
    //  iterate through all of the slider objects properties
    window.SliderProps = this;
    // set slider to be accessible to the global scope
  });
  // slider props stored as vars
  var $liderHook = SliderProps.settings.envokeBecause;
  var slideRail = SliderProps.bindings.slideRail;
  var slide = SliderProps.bindings.slide;
  var play = SliderProps.bindings.playButton;
  var next = SliderProps.bindings.nextButton;
  var prev = SliderProps.bindings.prevButton;
  var pause = SliderProps.bindings.pauseButton;
  var stop = SliderProps.bindings.stopButton;
  var slideCount = SliderProps.bindings.slideRail.children().size();
  var slideCounter = 0;

  function slidePrev(){
    var slidePrev = SliderProps.methods.slidePrev();
  }
  function slideStop(){
    /*slideRail.stop(); */
    window.clearInterval( $liderTimer  )
  }
  function slideRestart(){
    var slidePrev = SliderProps.methods.slideRestart();
    slideRail.stop();
  }
  function autoPlay(){
    SliderProps.methods.slideStart();
  }
  function slideNext(){
    slideCounter++
    var slideNext = SliderProps.methods.slideNext();
    console.log(slideCounter)
    console.log(slideCount)
    if (slideCounter != slideCount){
      // offset var-1 for js index compensation
      console.log('keep slidin..')
    }
    else {
      console.log('stop!')
      slideRestart();
      slideCounter = 0;
    }
  }
  // element -> event delegation type -> function()
  slideRail.hover( slideStop )
  next.click( slideNext )
  prev.click( slidePrev )
  stop.click( slideRestart )
  pause.click( slideStop )
  play.click( autoPlay )
} // close function slider()
$lider(true, 750);
});

I'd suggest taking a look at jquery-boilerplate, especially the jqueryUI widget portion.

I started refactoring your code in a true $widget manner. It's not completely functional but should point you in the right direction.

;(function ( $, window, document, undefined ) {
  $.widget("yourCustomNamespace.slider", {

    /*
     * Options to be used as defaults:
     */

    options: {
      autoplay: true,
      controls: true,
      duration: 750,
      slideSize: '100%',
      selectors: {
        slideRail: '[data-function~="slide-rail"]',
        slide: '[data-function~="slide"]',
        next: '[data-function~="next"]',
        prev: '[data-function~="prev"]',
        play: '[data-function~="play"]',
        pause: '[data-function~="pause"]',
        stop: '[data-function~="stop"]'
      }
    },

    /*
     * Private Methods:
     */

    _create: function() {
      this._setOption("current", 0);
      this._setOption("total", this.element.children().length);
      this._on(this.element, {
        "hover " + this.options.selectors.slideRail: $.proxy(this.stop, this),
        "click " + this.options.selectors.next: $.proxy(this.next, this),
        "click " + this.options.selectors.prev: $.proxy(this.prev, this),
        "click " + this.options.selectors.stop: $.proxy(this.stop, this),
        "click " + this.options.selectors.pause: $.proxy(this.pause, this),
        "click " + this.options.selectors.play: $.proxy(this.play, this)
      });
      if(this.options.autoplay) {
        this.start();
      }
    },
    _destroy: function() {
      this._off(this.element, "click hover");
    },
    _setOption: function(key, value) {
      switch (key) {
        case "current":
          // Update internal object ...
          this.options[key] = value;
          // Update DOM ...
          // this.element.find(this.options.selectors.slideRail.animate({...}, this.options.duration) ...
          break;
        default:
          this.options[key] = value;
          break;
      }
      return this._super("_setOption", key, value);
    },

    /*
     * Public Methods:
     */

    next: function() {
      // TODO: Account for overflow, this.options.total
      this._setOption("current", this.options.current + 1);
    },
    prev: function() {
      // TODO: Account for underflow, this.options.total
      this._setOption("current", this.options.current -1);
    },
    restart: function() {
      this._setOption("current", 0);
    },
    start: function() {
      this.interval = window.setInterval($.proxy(this.next, this), this.options.duration);
    },
    stop: function() {
      window.clearInterval(this.interval);
    },
    getCount: function() {
      return this.options.total;
    }
  });
})( jQuery, window, document );

Reference

There are a couple of things regarding reusability going on in here:

options are your default parameters that can be overwritten on initializing a new widget instance, e.g. $('[data-widget~="slider"]').slider({autoplay: false}); would overwrite the default of autoplay: true
Event binding is handled internally via this._on()
Ideally the widget would manage its own application state logic via this._setOption(). That way you can distinguish between logic and DOM interaction. Whenever a value changes, you update the DOM accordingly.
You can call public methods from other places via the automatic interface the widget provides, e.g. $('[data-widget~="slider"]').slider("next")


Prime factors of a number in C The following is code on "prime factors of a number". Any suggestions to optimize the code? It runs for a pretty long time for inputs like 35068499. I also want the code to take 10 digit numbers as input.

#include<stdio.h>
void pf(long);
void main()
{
    printf("Enter a number: ");
    scanf("%ld",&n);
    pf(n);
}
void pf(long n)
{
    long i,x;
    while(n%2==0)
    {
        printf("2\n");
        n=n/2;
    }
    x=3;
    while(x<=n)
    {
        for(i=2;i<x;i++)
        {
            if(x%i==0)
            {
                goto abc;
            }
        }
        while(n%x==0)
        {
            printf("%ld\n",x);
            n=n/x;
        }
        abc:
            x=x+2;
    }
}

Can you describe a bit your algorithm ? There is no comments in your code and it is not straightforward (because no comments is acceptable when the code is easy to read, but not in this case).
A few things that you should consider :

the use of global variables is here totally inapropriate, n should not be global and your function pf should take an argument (and should also return an array of the prime factors found, if you need them)
the while(1) is useless the two times you used it. It is commonly bad practise to use this, and especially when there is only one condition in the loop and when you exit the loop when the condition is not true. You should use while(condition) instead
Name correctly your variables ! What are i, x, n ? It improvs readability by a lot with absolutely no cost, so you should do it
I also don't see why you need the goto. Again it makes your code hard to read for no obvious good reason.
If n is a positive number, you should use an unsigned type. Since you want to be able to process 10 digits numbers, I would recommend using unsigned long long

Aside from that, I think it's mostly the algorithm that you use that is ineffective, which is why you should explain what you tried to do.


UPenn Homework 3: localMaxima :: [Integer] -> [Integer] Please see this question for the general description.
Exercise 2: localMaxima :: [Integer] -> [Integer]
A local maximum
of a list is an element of the list which is strictly
greater than both the elements immediately before and after it
localMaxima :: [Integer] -> [Integer]
localMaxima l =
  let getMaxOf3 x y z | x < y && y > z = Just y
                          | otherwise = Nothing
      accumulate l2 acc = case l2 of
                           u:v:w:xs -> case (getMaxOf3 u v w) of
                                        Just q -> accumulate (v:w:xs) (q:acc)
                                        Nothing -> accumulate (v:w:xs) acc
                           otherwise -> []
  in reverse (accumulate l [])

The implementation can be much simpler.  The main insight I can offer is to use map and filter instead of Maybe and writing your own accumulate.  In addition,

Instead of writing case l2 of …, use pattern matching.
Look for library functions that you can take advantage of.  (Here, I've used tails from Data.List.)
In keeping with the "wholemeal programming" goal, use higher-order functions instead of recursion.
Higher-order functions also let you use point-free style, which lowers your mini-golf score.

I suggest using where instead of let … in … because the former encourages top-down strategic thinking.  You want to write a "one-liner", and add supporting details only if necessary.

import Data.List (tails)

localMaxima :: [Integer] -> [Integer]
localMaxima = map (!!1) . filter isLocalMax . tails
  where
    isLocalMax (a:b:c:_) = a < b && b > c
    isLocalMax _         = False

In addition to being shorter, this implementation is more maintainable.  You can tell at a glance what map (!!1) . filter isLocalMax . tails does, but reverse (accumulate l []) is mysterious.  Furthermore, because isLocalMax is not intertwined with accumulate, you can trace the execution by inspecting tails xs, then (filter isLocalMax . tails) xs, etc.


Storing the data in 18 independent variables is awkward.  The naming of your variables suggests that each group of six doubles represents an object, which I've called a Sexdoublet.

Throwing all the code into main() is poor practice; there should be a function to do the importing.  It should accept an istream parameter and return a vector of Sexdoublets.

I don't know what you were hoping to accomplish with if (!ifs) ifs >> trash; — if the ifstream is bad then you shouldn't try reading from it.

Hard-coding the filename is probably a bad idea.  I've used the convention of reading from either a file that is specified on the command line, or from standard input otherwise.

#include <fstream>
#include <iostream>
#include <string.h>
#include <vector>

/* A struct of six doubles */
struct Sexdoublet {
    double x, y, d, m, c, t;

    friend std::istream &operator>>(std::istream &in, Sexdoublet &r) {
        return in >> r.x >> r.y >> r.d >> r.m >> r.c >> r.t;
    }

    friend std::ostream &operator<<(std::ostream &out, const Sexdoublet &r) {
        return out << "[ x = " << r.x
                   << ", y = " << r.y
                   << ", d = " << r.d
                   << ", m = " << r.m
                   << ", c = " << r.c
                   << ", t = " << r.t << " ]";
    }
};

std::vector<Sexdoublet> import(std::istream &in) {
    std::vector<Sexdoublet> data;
    Sexdoublet s;
    while (in >> s) {
        data.push_back(s);
    }
    return data;
}

int main(int argc, char *argv[]) {
    // If the first command-line argument is not "-", treat it as the filename
    // from which to read the input.  Otherwise, read from STDIN.
    const char *filename = (argc >= 2 && 0 != strcmp("-", argv[1])) ?
                                argv[1] : NULL;
    std::ifstream f;
    std::istream &in = filename ? (f.open(filename), f) : std::cin;
    if (!f) {
        std::cerr << "Error opening " << filename << ": "
                  << strerror(errno) << std::endl;
        return 1;
    }

    std::vector<Sexdoublet> data = import(in);
    std::for_each(data.begin(), data.end(), [](const Sexdoublet &s) {
        std::cout << s << std::endl;
    });
    return 0;
}


Splice-merge two sorted lists in Python I tried to solve a  merge two sorted list problem in leetcodes:

Merge two sorted linked lists and return it as a new list. The new list should be made by splicing together the nodes of the first two lists.
Example:
Input: 1->2->4, 1->3->4
Output: 1->1->2->3->4->4

class Solution:
    def mergeTwoLists(self, l1: ListNode, l2: ListNode) -> ListNode:
        """
        Plan:
        Compare l1 and l2 and merge the remainders
        """
        l3_head = ListNode(0)
        l3_cur = l3_head

        while l1 and l2: #assert both exist
            if l2.val < l1.val:
                l3_cur.next = l2 #build l3's node
                l2 = l2.next #this is i++
            else:
                l3_cur.next = l1
                l1 = l1.next
            l3_cur = l3_cur.next #find the next to build
        if l1:
            l3_cur.next = l1
        if l2:
            l3_cur.next = l2

        return l3_head.next

I assumed this is a decent solution but got:

Runtime: 56 ms, faster than 23.98% of Python3 online submissions for Merge Two Sorted Lists.
Memory Usage: 13.1 MB, less than 5.06% of Python3 online submissions for Merge Two Sorted Lists.

How could I improve my solution?

You shouldn't need to create a HeadNode, just use one of the ones given to you. Right now you relink every single item. But if one list has many sequential values, you could save time by just advancing to the tail.  Also, right now you are checking 2 conditions in every loop even though only 1 changes.

You could try something like this: (after setting cur to the minimum node):

while True:
   while cur.next and cur.next.val <= l2.val:
       cur = cur.next
   cur.next = l2
   if not cur.next:  break

   while cur.next and l2.val <= l1.val:
       cur = cur.next
   cur.next = l1
   if not cur.next:  break


Array-based stack data structure in Java I've recently been writing simple data structures, even though they exist in the library it helps me understand them a lot better.

public class Stack<E>
{
    private int capacity;
    private E[] data;
    private int top;

    /**
     * Default Constructor where the stack size is initially
     * set to 1000 elements.
     */
    public Stack(int capacity) {
        this.capacity = capacity;
        this.data = (E[])new Object[capacity];
        top = -1;
    }
    public Stack() {
        this(1000);
    }

    public int size() {
        return top + 1;
    }

    public boolean isEmpty() {
        return top == -1;
    }

    public E top() throws EmptyADTException {
        if(isEmpty())
            throw new EmptyADTException("Nothing in the stack!");
        return data[top];
    }

    public void push(E obj) {
        if(data.length == size())
            throw new IllegalStateException("Stack is full!");
        data[++top] = obj;
    }

    public E pop() throws EmptyADTException {
        if(isEmpty())
            throw new EmptyADTException("Nothing in the stack!");
        E popObj = data[top];
        data[top--] = null;
        return popObj;
    }
}

I know that there are drawbacks to using an array-based stack, because of the limited size. Although, in the long run, it is more efficient if the time per operation isn't an issue, when compared to a linked-list implementation. I've also heard of people now using something called a VList.

I was wondering, is there any way I can improve this code whilst still using an array-based stack?

Auto-resize.

There's nothing saying an array-based stack has to have a fixed size. The standard library Stack doesn't have a fixed size, and it's array-based. Same with ArrayDeque, the stack you should be using instead of the Stack class.

Just make a bigger array if you run out of room. You'll want to grow the array by a multiplicative factor to make sure you don't have to resize too often; you want push to be amortized constant time.

public void push(E obj) {
    if(data.length == size())
        data = Arrays.copyOf(data, 2*data.length+2); // +2 in case data.length == 0
    data[++top] = obj;
}

Get rid of the capacity instance variable.

You literally never use it.

Are those checked exceptions?

You declare throws EmptyADTException on two of your methods, which you shouldn't have to do. throws is only necessary for checked exceptions, and EmptyADTException shouldn't be a checked exception. Checked exceptions are for things that might go wrong even if the code is correct, like FileNotFoundException. If someone tries to pop from an empty stack, that's a coding error.

If your EmptyADTException is a checked exception, make it descend from RuntimeException so it's unchecked. Making it a checked exception just bloats any code that uses your stack, and the exception-swallowing catch blocks that result will actually reduce the chances that bugs are found.

Consider throwing NoSuchElementException.

It's what the Java Collections Framework throws for cases like popping from an empty stack. There's also EmptyStackException, which is what the standard library Stack throws, but I don't recommend it. Stack is old, doesn't really fit with newer additions to the Java standard library, and has old design mistakes like built-in synchronization. You shouldn't try to copy it.

Don't use the increment/decrement operators inside of a bigger expression.

Code like data[++top] = obj; requires more thought about what happens in what order than something like ++top; data[top] = obj;.

Don't try to make generic arrays.

    this.data = (E[])new Object[capacity];

This might look like it works, but that's just because type erasure prevents Java from noticing the error. What you're doing here is storing an Object[] in a variable of type E[], even though Object probably isn't E. If you ever do anything that lets Java notice the error, for example

// Inside the Stack class, so data is accessible
Integer[] ints = new Stack<Integer>(5).data;

you'll get a ClassCastException. The correct way to make a generic array is a lot of unnecessary hassle; the best way to go is generally to just make the data variable an Object[]:

public class Stack<E>
{
    private int capacity;
    private Object[] data;

and cast elements when you retrieve them from the array:

public E top() throws EmptyADTException {
    if(isEmpty())
        throw new EmptyADTException("Nothing in the stack!");
    return (E) data[top];
}

For reference, this is how you'd make a generic array correctly:

public Stack(Class<E> klass, int capacity) {
    data = Array.newInstance(klass, capacity);
    ...

and then you'd need to pass a Class object into the Stack constructor:

Stack<Integer> stack = new Stack<>(Integer.class, 10);


Count swaps needed to order a list of integers I wrote this code since I'm going through the hackerrank challenges, however, it takes way too long to run.
Its function is described on the title, but just to clear it up, we need to count the minimum amount of swaps needed to order a set of ascending integers.

def minimumSwaps(arr):
    count = 0
    correct = [(x+1) for x in range(len(arr))]
    while arr != correct:
        for i in arr:
            if i != arr.index(i)+1:
                a = i
                b = arr[i-1]
                arr[i-1] = a
                arr[arr.index(i)] = b
                count += 1
                break
    return count
if __name__ == '__main__':
    n = int(input())
    arr = list(map(int, input().rstrip().split()))
    res = minimumSwaps(arr)
    print(str(res))

I don't have time to really look at the code well or do a full review, but what did stick out to me was the repeated calls to arr.index(i). That's an expensive call that will require, in the worst case, a full iteration of arr each time.

At the very least, that call should be done once then cached. Really though, that information is already available to you without searching if you use enumerate:

def minimum_swaps(arr):
    count = 0
    correct = [(x+1) for x in range(len(arr))]
    while arr != correct:
        for i, x in enumerate(arr):  # I changed what was i to x, then made i the index
            if x != i+1:
                a = x
                b = arr[x-1]
                arr[x-1] = a
                arr[i] = b
                count += 1
                break

    return count

I also renamed some variables. i is an awful name for an element that isn't itself an index, and the function name should be in snake_case instead.


Match all youtube links in a string of text I'm looking to pull all the youtube links from a string of text and was wondering how does this look?

if (preg_match_all('/(https?:\/\/)?(www\.)?(youtube\.com)\/watch\?v=([a-zA-Z0-9_-]+)[^\s]*/im', $this->content, $matches)) {

}

Obviously this doesn't take into account the youtu.be links as they are formatted differently.

[^\s] is effectively just \S
For checking against youtu.be links, you can have:

youtu(\.be|be\.com)

If you do not want to store every matched group, use (?:...)
[A-Za-z0-9_] can be simplified to \w.
Use a different delimiter than / so that you won't have to escape it in pattern.

Therefore:

preg_match_all(
    '@(https?://)?(?:www\.)?(youtu(?:\.be/([-\w]+)|be\.com/watch\?v=([-\w]+)))\S*@im',
    $this->content,
    $matches
)


Filtering out, a list of games, based on their followers so I am playing with Rawg.io 's API.
U can sort their game list by their release date, however there are just too many games. In their website, there's an option called "Popular only", which only shows games that are being followed by at least 1 people.
I implemented this feature by sorting the elements I get and creating a sorted list. I wanted to make sure that I would also acces their information in a systematically way ([game's number], [name or other topic]).
(the ##delete## parts are parts that are just for console visuals, I am planning to move this project to a Kivy app, so won't need them later)
I really want to know what I could have done differently. What's obnoxious, stupid,  ... could be better?
import requests

######Delete#########
import os

os.system("cls")
###################

#### Functions
def _Url(year1, month1, day1, year2, month2, day2, PageNumber=1):
    if month1 < 10:
        month1 = "0" + str(month1)
    if day1 < 10:
        day1 = "0" + str(day1)
    if month2 < 10:
        month2 = "0" + str(month2)
    if day2 < 10:
        day2 = "0" + str(day2)

    Url = (
        "https://api.rawg.io/api/games?dates="
        + str(year1)
        + "-"
        + str(month1)
        + "-"
        + str(day1)
        + ","
        + str(year2)
        + "-"
        + str(month2)
        + "-"
        + str(day2)
        + "&page="
        + str(PageNumber)
        + "&platforms=4,187,1,18,186&page_size=40&ordering=released"
    )
    return Url

def _GetData_Date(year1=2020, month1=8, day1=4, year2=2020, month2=8, day2=4): #just place holder values

    Url = _Url(year1, month1, day1, year2, month2, day2)
    StaticData = requests.get(Url).json()
    AmountOfGames = StaticData["count"]
    AmountOfPages = AmountOfGames // 40 #1 page contains 40 so it does amount//40 to get pages
    if(AmountOfGames%40 != 0):
        AmountOfPages = AmountOfPages + 1 #does +1 if there's a leftover page
    PageNumber = 1

#############Delete############
    Succes = 0
    Fail = 0
#########################

    while PageNumber <= AmountOfPages:

        for x in range(len(StaticData["results"])):
            if StaticData["results"][x]["added"] > 0:
                try:   #can't start with an empty Game variable
                    AStaticData = (StaticData["results"][x],)  # the ","turns it to tulpe, it keeps adding tulpes to 1 giant one that contains all the filtered information
                    Game = Game + AStaticData
                except:
                    AStaticData = (StaticData["results"][x],)  # the ","turns it to tulpe
                    Game = AStaticData
############################Delete#################
                os.system("cls")
                Succes = Succes + 1
                print("succes: ", (Succes))
                print("fail:", Fail)
                print("Total:", AmountOfGames, Succes + Fail)
            else:
                os.system("cls")
                Fail = Fail + 1
                print("succes: ", (Succes))
                print("fail:", Fail)
                print("Total:", AmountOfGames,"-", Succes + Fail)
##################################################
        # goes trough pages
        PageNumber = PageNumber + 1
        if PageNumber <= AmountOfPages:
            Url = _Url(year1, month1, day1, year2, month2, day2, PageNumber)
            StaticData = requests.get(Url).json()

    return Game

#### Save it to local variables #####
StaticData =_GetData_Date()

### Main program ###################
for x in range(len(StaticData)):
    print(StaticData[x]["name"])

f-strings
I recommend to use f-strings (Python version >= 3.6) for string formatting. See this link for more details:
https://www.python.org/dev/peps/pep-0498/
When you use them you can replace
if month1 < 10:
    month1 = "0" + str(month1)

with an expression like this:
month1 = f'{month1:02}'

This adds a leading zero if month1 only has one digit.
In addition, an expression like:
 Url = (
        "https://api.rawg.io/api/games?dates="
        + str(year1)
        + "-"
        + str(month1)
        + "-"
        + str(day1)
        + ","
        + str(year2)
        + "-"
        + str(month2)
        + "-"
        + str(day2)
        + "&page="
        + str(PageNumber)
        + "&platforms=4,187,1,18,186&page_size=40&ordering=released"
    )

can be written as
url_part1 = "https://api.rawg.io/api/games?dates="
url_part2 = "&platforms=4,187,1,18,186&page_size=40&ordering=released"

 Url = f"{url_part1}{year1}-{month1}-{day1},{year2}-{month2}-{day2}&page={PageNumber}{url_part2}"

using f-strings. This is shorter and less error prone (less quotes you can forget), opinions might vary regarding the readability, though. Type conversions (str(.)) are not needed anymore.
Avoid hardcoding
It is good that you added a comment to explain where the number 40 comes from:
AmountOfPages = AmountOfGames // 40 #1 page contains 40 so it does amount//40 to get pages

However, it would be even better if you assigned this number to a variable with a telling name. For example
games_per_page = 40 # Preferably, put this on the top of your script or in a config file
# [...]
AmountOfPages = AmountOfGames // games_per_page

This is more flexible. Imagine the number of games per page changes.
Leftover page: ceiling division
In this part you first do a floor division (//), then you add 1 for leftover pages
AmountOfPages = AmountOfGames // games_per_page#1 page contains 40 so it does amount//40 to get pages
if(AmountOfGames%games_per_page != 0):
    AmountOfPages = AmountOfPages + 1 #does +1 if there's a leftover page

This can be simplified with a ceiling division
import math

AmountOfPages = math.ceil(AmountOfGames / games_per_page)


File uploader in php I can't get a peer review where I work, I was wondering how my code was and how could it be improved.

https://github.com/lamondea/uploader

Is it a good practice to use functions in classes or should it be replaced by external function that use the object returned by the class:

class _export
{
    public $nbr;
    public $file;

    public function __construct($file,$nbr=0){
        if($nbr != 0 && ($file->ext == 'jpg' || $file->ext == 'png' || $file->ext == 'gif'))
            thumbnailer($file,$nbr);
        else {
            move_uploaded_file($file->tmp_name, ($file->loc).($file->name));
            return $file->name;
        }
    }
}

I'm not sure if using classes is better or worse but I am starting to see files being called as functions more than a single file with function within. This is a more common upload script.

$allowedExts = array("gif", "jpeg", "jpg", "png");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);

if ((($_FILES["file"]["type"] == "image/gif")
    || ($_FILES["file"]["type"] == "image/jpeg")
    || ($_FILES["file"]["type"] == "image/jpg")
    || ($_FILES["file"]["type"] == "image/pjpeg")
    || ($_FILES["file"]["type"] == "image/x-png")
    || ($_FILES["file"]["type"] == "image/png"))
    && ($_FILES["file"]["size"] < 20000)
    && in_array($extension, $allowedExts)){
    if ($_FILES["file"]["error"] > 0){
        echo "Error: " . $_FILES["file"]["error"] . "
";
    }
    else{
        echo "Upload: " . $_FILES["file"]["name"] . "
";
        echo "Type: " . $_FILES["file"]["type"] . "
";
        echo "Size: " . ($_FILES["file"]["size"] / 1024) . " kB
";
        echo "Stored in: " . $_FILES["file"]["tmp_name"];
    }
}
else{
    echo "Invalid file";
}


String checking functions in functional JavaScript Please review these string checking functions. This is the whole module
which is actually written here. It supports buffer, array, and
string by checking the char code.

const charCode = chr => typeof chr === 'number' ? chr : chr.charCodeAt(0)

const every = (fn) => (list) => {
  for (let i = 0; i < list.length; ++i)
    if (!fn(list[i])) return false
  return true
}

const compose   = (f, g) => a => f(g(a))
const inBetween = (num, min, max) => num >= min && num <= max

const space        = code => code == 9  || code == 10 || code == 12 ||
                             code == 13 || code == 32
const numeric      = code => inBetween(code, 48, 57)
const lowerAlpha   = code => inBetween(code, 97, 122)
const upperAlpha   = code => inBetween(code, 65, 90)
const alpha        = code => lowerAlpha(code) || upperAlpha(code)
const alphaNumeric = code => numeric(code)    || alpha(code)
const printable    = code => space(code)      || inBetween(code, 32, 127)

My thought is is that this style is less readable than the original one. What do you think about its readability? And do these functions satisfy what
said as functional?

I think these are rather readable, actually more so than the original ones. You've condensed the functions considerably!

Going pointfree

One further possibility would be to go more "pointfree", which you find a lot of in Haskell, for example. Define or in terms of functions (so its arguments can be evaluated lazily), then you can work at a higher level of abstraction:

const or = (f, g) => (...args) => f.apply(f, args) || g.apply(g, args)

const alpha        = or(lowerAlpha, upperAlpha)
const alphaNumeric = or(numeric, alpha)
const printable    = or(space, inBetween(32, 127))

And then you can configure inBetween to accept the variable parameter separately:

const inBetween  = (a, b) => x => a <= x && x <= b

const space      = x => [9, 10, 12, 13, 32].includes(x)
const numeric    = inBetween(48, 57)
const lowerAlpha = inBetween(97, 122)
const upperAlpha = inBetween(65, 90)

I find this a bit more readable, since it removes the repetition of the parameter code. The resulting code is denser and reads nicely: "alpha means either lowerAlpha or upperAlpha", etc.

But really this comes down to your preferences. As far as

  do these functions satisfy what said as functional?

that depends on your definitions/requirements. I'd say they're certainly functional in the sense of avoiding mutability, except for every, and in the philosophy of composing together small functions.

every

A more functional every might recurse while fn(x) is truthy and elements remain in the list, something like this:

const every = (pred, [head, ...tail]) =>
    pred(head) && (tail.length ? every(pred, tail) : true)

but this won't behave nicely in current JS engines. One modification you could do now, though, is to use for...of in every, so that it also works on Sets, ES7 generators, and other iterables that might not be indexable:

const every = pred => list => {
    for (let x of list) {
        if (!pred(x)) return false
    }
    return true
}


Find-and-replace optimization I'm looking for some JavaScript code improvement and optimization. Is it possible to optimize this block of code (for example, remove double array loop)?

replace: function(ex, str) {
    var arr = str.match(/{add_([A-Z]+)}/gi);
    if (arr && arr.length) {
       arr.each(function(e){
          var id = e.replace('{add_','').replace('}',''),
          o = Core.get_data_by_id(ex.extensions, id);

          if (o && o.status =="1") {
            str = str.replace('{add_'+id+'}', o.html);
          }
       });
    }

    var arr2 = str.match(/{banner_([0-9]+)}/gi);
    if (arr2 && arr2.length) {
       arr2.each(function(e){
         var id = e.replace('{banner_','').replace('}',''),
         b = Core.get_data_by_id(ex.banners, id);

         if (b && b.status =="1" && b.code && b.code.length) {
            var content = '

' +
            b.code.each(function(s){
               content += '
'+s.code+'
';
            });
            content += '

';
            str = str.replace('{banner_'+id+'}', content);
         }
       });
    }

    return str;
}

You should be using String.prototype.replace() with a function as the second argument. Example:

str = str.replace(/{add_([A-Z]+)}/gi, function(match, id) {
  var o = Core.get_data_by_id(ex.extensions, id);
  return (o && o.status == "1") ? o.html : match;
});

str = str.replace(/{banner_([0-9]+)}/gi, function(match, id){
  var b = Core.get_data_by_id(ex.banners, id);

  if (b && b.status =="1" && b.code && b.code.length) {
    var content = '

' +
    b.code.each(function(s){
       content += '
'+s.code+'
';
    });
    content += '

';
    return content;
  }
  return match;
});

EDIT: I've added the second part.


Broken keyboard challenge Inspired by Reddit r/dailyprogrammer

  Help! My keyboard is broken, only a few keys work any more. If I tell you what keys work, can you tell me what words I can write?

The program works fine, not really any additions I wish to make to it. Was just wanting some general feedback on the layout, structure and efficiency.

Note: The dictionary used can be found in the given link.

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.List;

/**
 * Created on 7/1/2016.
 *
 * Inspired by r/dailyprogrammer
 * https://www.reddit.com/r/dailyprogrammer/comments/3pcb3i/20151019_challenge_237_easy_broken_keyboard/
 */

public class BrokenKeyboard {

    private static List<String> wordDict;
    private static final String dictPath = "Dictionaries/EnglishWords.txt";

    /**
     * The main acquires input from the user then
     * applies it to the other below methods.
     * @param args Unused.
     */

    public static void main(String args[]) {
        BrokenKeyboard.setDict(dictPath); // Stores dictionary in list.
        System.out.println("Processing...");
        System.out.println(String.join(", ", findMatches("bikn", wordDict)));
    }

    /**
     * Used to load a given text file into memory.
     * @param filePath Used to specify the path of the file that needs to be loaded into memory.
     */
    private static void setDict(String filePath){
        try {
            wordDict = Files.readAllLines(new File(filePath).toPath());
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    /**
     * Used to compare a given key to the text file loaded into memory.
     * @param key The inputted set of characters compared to the given dictionary.
     * @param dict The list of words that will be used to compare to the key.
     * @return The list of matches made when comparing the key with the given dictionary.
     */
    private static List<String> findMatches(String key, List<String> dict){
        List<String> matchList = new ArrayList<>();
        for(String word : dict){
            String temp = word; // Stores word so that it can be manipulated.
            for(char character : key.toCharArray()) {
                temp = temp.replace(Character.toString(character), "");
            }
            if(temp.equals("")){
                matchList.add(word);
            }
        }
        return matchList;
    }
}

setDict() is a bit of a misnomer; loadDict() would be more accurate.  Since the loaded dictionary becomes part of the global state, it would be more appropriate to make it an instance variable than a static variable.  If loading fails, you shouldn't just print a stack trace and attempt to continue execution — that defeats the purpose of exceptions.  In general, if you don't have a good way to handle an exception, just propagate it (by declaring throws IOException).

Your technique for checking whether a word consists solely of a limited set of characters is to do repeated string replacement.  That would cause a lot of temporary objects to be allocated and discarded.  A more appropriate tool to use would be a regular expression match (using Pattern.compile("^[bikn]*$"), for example).

Since this is a filtering exercise, it would be more eloquently written using Stream<String>.filter(Predicate) instead of List<String>.

Although String args[] is syntactically valid, String[] args is considered more idiomatic in Java, since String[] is considered the type.

Suggested solution

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.function.Predicate;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class BrokenKeyboard {
    public static Predicate<String> typableUsing(String availableKeys) {
        // CAVEAT: availableKeys is assumed not to contain regex metacharacters
        // such as ^ [ ] - \
        return Pattern.compile("^[" + availableKeys + "]*$").asPredicate();
    }

    public static Stream<String> dictionary(File f) throws IOException {
        return Files.lines(f.toPath());
    }

    public static void main(String[] args) throws IOException {
        System.out.println(
            BrokenKeyboard.dictionary(new File(args[1]))
                          .filter(BrokenKeyboard.typableUsing(args[0]))
                          .collect(Collectors.joining(", "))
        );
    }
}

It is possible to resolve the caveat using Pattern.quote().  I'll leave it as an exercise to the reader.  =)


Reverse Message Program # Alex Grattan
# 17/10/14

message = input("Enter your message to have it printed backwards:")
split_message = list(message.split())
reverse_message_list = []
reverse_message = ""

for i in range(len(split_message)):
    reverse_message_list.append(split_message.pop())

reverse_message_list = [x + ' ' for x in reverse_message_list]

for i in reverse_message_list:
    reverse_message += i

print (reverse_message)

split returns a list already, so you don't need the list call
unless you really want to reimplement the reverse method, use it, or the function reversed if you want a return value instead
concatenating strings is much easier with " ".join(...)

So this can then be compressed to

# Alex Grattan
# 17/10/14

message = input("Enter your message to have it printed backwards:")
print (" ".join(reversed(message.split())))


Writing Traverse Instance for Option Working on an exercise from Functional Programming in Scala, I implemented a Traverse instance for Option:

override def traverse[G[_],A,B](oa: Option[A])(f: A => G[B])(implicit G: Applicative[G]):
    G[Option[B]] =
  oa match {
    case None => G.unit(None)
    case _ => {
              val a: A = oa.get
              val x: G[B] = f(a)
              G.map(x)(Some(_))
            }
  }

Is this idiomatic? Besides condensing the case _'s 3 lines to 1, perhaps there's a more concise way to write this method?

Never use get method on option. In this case you could use case Some(a) instead.
You don't need curly braces in case branches

oa match {
  case None => G.unit(None)
  case Some(a) =>
    val x: G[B] = f(a)
    G.map(x)(Some(_))
}

I don't know if variable x makes any sense in this case. I'd just use this:

case Some(a) => G.map(f(a))(Some(_))

You could also use methods of Option like fold of map + getOrElse, but I guess pattern matching is the best solution in this case.


Check for balanced parentheses Given an expression string exp, write a program to examine whether the
  pairs and the orders of

"{","}","(",")","[","]"

  are correct in exp.

  For example, the program should print true for

exp = "[()]{}{[()()]()}"

  and false for

exp = "[(])"

Complexity:

Time complexity: O(n) where n is length of the string
Space complexity: O(n/2) where n is length of string

Looking for code review, optimizations and best practices.

public final class BalancedParanthesis {

    private static final Map<Character, Character> brackets = new HashMap<Character, Character>();
    static {
        brackets.put('[', ']');
        brackets.put('{', '}');
        brackets.put('(', ')');
    }

    private BalancedParanthesis() {};

    /**
     * Returns true is parenthesis match open and close.
     * Understands [], {}, () as the brackets
     * It is clients responsibility to include only valid paranthesis as input.
     * A false could indicate that either parenthesis did not match or input including chars other than valid paranthesis
     *
     * @param str   the input brackets
     * @return      true if paranthesis match.
     */
    public static boolean isBalanced(String str) {
        if (str.length() == 0) {
            throw new IllegalArgumentException("String length should be greater than 0");
        }
        // odd number would always result in false
        if ((str.length() % 2) != 0) {
            return false;
        }

        final Stack<Character> stack = new Stack<Character>();
        for (int i = 0; i < str.length(); i++) {
            if (brackets.containsKey(str.charAt(i))) {
                stack.push(str.charAt(i));
            } else if (stack.empty() || (str.charAt(i) != brackets.get(stack.pop()))) {
                return false;
            }
        }
        return true;
    }

    public static void main(String[] args) {
        assertEquals(true, isBalanced("{}([])"));
        assertEquals(false,isBalanced("([}])"));
        assertEquals(true, isBalanced("([])"));
        assertEquals(true, isBalanced("()[]{}[][]"));
    }
}

I just wanted to write that the code looks completely fine to me, until...
assertEquals(false, isBalanced("[["));

I''m sure you'll find the error :-)
Otherwise two small remarks:

Personally I'd have the function return true for an empty string instead of throwing an exception.

Stack is outdated. I'd use a Deque implementation instead.

EDIT: I'm sorry, but I just realized something else: Your program doesn't fulfill the requirement O(n/2) for space complexity.


Displaying a single record based on DocID I have a table with ~900,000 records. I need to display a single record based on DocID using this query. The record is the latest record that got modified by another program.

This piece of query run ~7 seconds:

SELECT        TOP 1 t.Register
FROM            PxStats AS s INNER JOIN Therapy AS t ON s.ID = t.ID
WHERE        (s.Date = DATEADD(dd, 0, DATEDIFF(dd, 0, GETDATE()))) AND (t.DocID = 1)
ORDER BY s.Time DESC

I do not have access to modify the tables design, the table is indexed by ID, but I can't filter the record based on ID because the latest iD is not guaranteed the latest record modified.

Starting with SQL 2008 there is a window function that does this nicely.

900,000 records even with lack of index that seems like it should be faster.

I doubt moving the condition into the join will fix the performance but it is worth a try.

SELECT TOP 1 t.Register
  FROM PxStats AS s
 INNER JOIN Therapy AS t
    ON s.ID = t.ID
   AND s.Date = DATEADD(dd, 0, DATEDIFF(dd, 0, GETDATE()))
   AND t.DocID = 1
 ORDER BY s.Time DESC;

If you can post the query plan.

If you remove the top 1 on average how many records would be returned?

A reach but try

declare @today datetime = (select DATEADD(dd, 0, DATEDIFF(dd, 0, GETDATE())));

SELECT TOP 1 t.Register
  FROM PxStats AS s
 INNER JOIN Therapy AS t
    ON s.ID = t.ID
   AND s.Date = @today
   AND t.DocID = 1
 ORDER BY s.Time DESC;


Hardware resource Open/Close methods So I'm making a program that is a serial port wedge. In other words I want it to listen to a serial port. If data comes in, wait a given amount of time, then play a sound and finally send the data out.

All of those things have a abstract class associated with it. I'm thinking that I might eventually want to monitor USB:HID things so I made a IInput class which has abstract methods of Open, Close, ToXmlString, ParseXmlString and has a event of DataAvailable. I extend that class with ComPortInput.

This is the question of it: Do I subscribe to my data event in the Open method, then unsubscribe in close? Or should I just have it in the initializer. I have a GUI that a user can change any of the 4 elements on this wedge at any time. But I'm not sure what would be the cleanest way of doing this. I'm thinking that the GUI would always create a new instance of my ComPortInput class. Part of me wants to force that GUI to have another method like Check Input. which in this case if the COM port was invalid or busy it would return false. Then instead of making a ComPortInput I would then want a NoInputInput class (for when I don't want to port forward) If I did it this way then I could get rid of my null checks on the serial port (Much desired) but is that too tricky? (as in too hard to read?) I'm thinking that if i do that I could put in my constructor of ComPortInput to Assert not null of the SerialPort. To me this looks better

public ComPortInput(SerialPort sp, int delay) : base(delay)
{
    if (sp == null)
        throw new System.ArgumentNullException("Serial port can not be null");

    LibraryTrace.Start("ComPortInput", delay);

    serialPort = sp;
    serialPort.DataReceived += sp_DataReceived;

    LibraryTrace.End("ComportInput");
}
~ComPortInput()
{
    serialPort.Dispose();
}

My thoughts on this way, though, is that it might be hard to have a abstract CheckValidity... Hmm just thought as I was typing what if I put a params object[] args for CheckValidity and just have the comport i could try a few things and return true or false. Then if I ever decide to accept other types of input I could do something similar (such as if it was USB:HID I could accept the PID/VID) What do you guys think? Am I on the right track?

As Jesse C. Slicer mentioned, because your class seems to encapsulate an IDisposable object (assuming serialPort would be a private field - this would be clearer if you named it _serialPort), it's asking to implement the IDisposable interface.

Thus, instead of a destructor, you would simply have a Dispose method:

public void Dispose()
{
    serialPort.Dispose();
}

Another thing is this LibraryTrace whose Start and End static methods are a testability hinderance. If it's at all possible, I would wrap that with an interface like ILibraryTrace and implement it in a LibraryTraceWrapper : ILibraryTrace class that exposes the static LibraryTrace methods as instance methods - and inject it into the constructor as a dependency - same goes for the SerialPort:

private readonly ILibraryTrace _trace;
private readonly ISerialPort _serialPort;

public ComPortInput(ILibraryTrace tracer, ISerialPort sp, int delay) : base(delay)
{
    // ...
}

This way you could unit test your class and inject mocks as needed.

As for your non-null assertion, that's called a guard clause and it's good because you're failing fast; by throwing in the constructor you thwart the instantiation of your class without a serial port.

I don't know if it's a typo or if it could be an actual issue, but LibraryTrace.Start and LibraryTrace.End are not working off the same string parameter value (you have "ComPortInput" and "ComportInput" - those are not the same string in C#). Given you're passing in the type's name, I would seriously consider passing in typeof(this).Name instead of a magic string. This shields both calls from an eventual rename refactoring.


Insert multiple rows without using CURSOR SQL Server 2014 I have a fairly complex query which insert multiple rows into a table from multiple tables. Its works fine but I really don't want to use cursor as some blog says cursor took long time for execution (of course it depends). I hope I could find alternative way for minimize my execution time or cursor complexity. Here is my query:

DECLARE @employeeid int;
DECLARE @joiningdate date;
DECLARE @quitdate date;
DECLARE @eodrefs int,@amounts decimal;

select @employeeid=HRMS_Employee.ID,@joiningdate=HRMS_Employee.Joining_Date,@quitdate=HRMS_Employee.QuitDate
from HRMS_Employee where HRMS_Employee.Present_Status=1 and ID=4

DECLARE @tabEodRecord table(eodref  int,amount decimal)
insert INTO @tabEodRecord (eodref,amount)

select HRMS_EodRecord.Eod_RefFk,HRMS_EodRecord.ActualAmount from HRMS_EodRecord join HRMS_Employee on HRMS_EodRecord.EmployeeId=HRMS_Employee.ID
where HRMS_EodRecord.Status=1 and HRMS_EodRecord.EmployeeId=@employeeid

IF CURSOR_STATUS('global','PayrollInfoCursor')>=-1
    BEGIN
     DEALLOCATE PayrollInfoCursor
    END

DECLARE PayrollInfoCursor CURSOR
FOR (select eodref,amount from @tabEodRecord)
OPEN PayrollInfoCursor;
    FETCH NEXT FROM PayrollInfoCursor INTO  @eodrefs,@amounts
    WHILE @@FETCH_STATUS = 0
BEGIN
insert HRMS_PayrollInfo (PayrollFK,EmployeeIdFk,Eod_ReferenceFk,Amount,PaymentDate,[Status],AlertActive,Entry_By,Entry_Date,Update_By,Update_Date)
select 26,@employeeid,@eodrefs,@amounts,getdate(),1,1,1,getdate(),1,getdate()
FETCH NEXT FROM PayrollInfoCursor INTO  @eodrefs,@amounts
END
CLOSE PayrollInfoCursor
DEALLOCATE PayrollInfoCursor

I hope there is something like this (however this won't work) because subquery returns multiple rows

insert HRMS_PayrollInfo (PayrollFK,EmployeeIdFk,Eod_ReferenceFk,Amount,PaymentDate,[Status],AlertActive,Entry_By,Entry_Date,Update_By,Update_Date)
select 26,@employeeid,(select eodref from tabEodRecord),(select amountfrom tabEodRecord),getdate(),1,1,1,getdate(),1,getdate()

From what I can tell based on the code you've presented, you can reduce it down to the following...

DECLARE @employeeid INT;

INSERT dbo.HRMS_PayrollInfo (PayrollFK, EmployeeIdFk, Eod_ReferenceFk, Amount, PaymentDate, Status, AlertActive, Entry_By, Entry_Date, Update_By, Update_Date)
SELECT
    26,
    @employeeid,
    er.Eod_RefFk,
    er.ActualAmount,
    GETDATE(),
    1,
    1,
    1,
    GETDATE(),
    1,
    GETDATE()
FROM
    dbo.HRMS_EodRecord er
    --JOIN dbo.HRMS_Employee e
    --  ON er.EmployeeId = e.ID
WHERE
    er.Status = 1
    AND er.EmployeeId = @employeeid;


Calculating kills and deaths in a game I'm trying to make a program that calculates your k/d ratio (kill/death, which is used in FPS games to make people believe that it's skill), how many kills you need without dying once to reach a goalKD. It also has a part that calculates how many battles you need if you give the program your average battle kills and battle deaths.

Is there a efficient way to program it? Currently, the way I'm doing it is getting me into programmer-efficiency hell.

For i = 1 to 100000 {
    While GoalKD>KDratio {
      Kills = BattleKills + Kill
      Deaths = BattleDeaths + Death
      KDratio = Kills / Deaths
      i++
  }
}

I'm programming this in SmallBasic because I'm most familiar with it, and I think it's easily readable. Also, if possible, don't give the answer right away; give me some hints for me to practice my mind. Add the answer in a spoiler box.

For the kills needed part, you are trying to solve this equation:

k{current} + n
--------------  = r
  d{current}

Where r is the target rate and n is the number you're looking for. Some basic algebra:

n = rd - k

For the battles part, you need to solve

k{current} + b * k{battle}            // current kills + additional kills
--------------------------  = r
d{current} + b * d{battle}            // current deaths + additional deaths

k{current} + b * k{battle} = r * (d{current} + b * d{battle})  // multiply both sides by d{current} + b * d{battle}

k{current} + b * k{battle} = r * d{current} + r * b * d{battle})  // just showing multiplication of r

b * (k{battle} - r * d{battle}) = r * d{current} - k{current}  // subtracting terms from each side

     r * d{current} - k{current}
b =  ---------------------------  // dividing by k{battle} - r * d{battle}
     k{battle} - r * d{battle}

In your code above, this becomes

need = Math.ceiling( GoalKD * Deaths - Kills )
battles = Math.ceiling( ( GoalKD * Deaths - Kills ) / (BattleKills - GoalKD * BattleDeaths) )

[I'm not a SmallBasic guy, so please forgive any syntax errors above.]

(Edited to fix math error re battles required and add comments to the math.)


Changing text with a click event: how to restore default state of text in jQuery if/else statement I have a click event that calls a function which makes some CSS changes (acts on Text in 3 unique text elements).

The function (below) works, but is lacking a 'restore default' function (to return the text to its original state), AND is in dire need of refactoring.

Q: How can the original state of ALL text elements be restored once user clicks away from this section?

Here is the partially working, ugly function:

function txtResize() {

        var clicked = this.id;

        if (clicked == "Text1")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text2, #Text3").css("opacity", "0.2");
        }
        else if (clicked == "Text2")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text1, #Text3").css("opacity", "0.2");
        }
        else if (clicked == "Text3")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text1, #Text2").css("opacity", "0.2");
        }
         // code above is ok, below does not work

        else if ( !(clicked == "Text1") && !(clicked == "Text2") && !(clicked == "Text3") )
        {
            $("#Text1, #Text2, #Text3").css("opacity", "1");  // return to default state
        }

 };

UPDATE: (20 Jan) - Have improved the code, but it still somewhat clunky and involves two distinct functions.

function txtResize() {

        var clicked = this.id;

        if (clicked === "Text1" || clicked === "Text2" || clicked === "Text3" )
        {
            $("#Text1, #Text2, #Text3").animate({"opacity": "0.2"}, 500)
            $("#" + clicked).animate({"opacity": "1"}, 200);
        }
};

function txtRestore(){
        $("#Text1, #Text2, #Text3").animate({"opacity": "1"}, 1000);
};

as a first step I would rewrite the last if statement

else if ((clicked =! "Text1") && (clicked != "Text2") && (clicked != "Text3") )
    {
        $("#ActText, #AdaptText, #PlanText").css("opacity", "1");  // return to default state
    }

After that you could rewrite it as a switch statement: http://www.w3schools.com/js/js_switch.asp

edit : this would be better...

var textArray = new Array("Text1","Text2","Text3");
if($.inArray(clicked,textArray){
    $("#Text2, #Text3").css("opacity", "1");
}
else {
 $("#" + clicked).css("opacity", "0.2");
}


Entering address details into Google Maps geocoder I have a Google Maps Geocoder and I am looking to refactor it. The user enters address details into a form where at the bottom there is the option to show on map.

First the map is hidden
The user clicks show_map checkbox
Map-area is shown
Map is initialized

The codeAddress() function is used if the user edits the address and wants to find it again. The showCoords() function passes the longitude and latitude through to the form for processing. I am looking for a best practice to refactor this code.

function initialize() {

    geocoder = new google.maps.Geocoder();

    codeAddress();

    map = new google.maps.Map(document.getElementById('map-canvas'));

    marker = new google.maps.Marker({
        map: map,
        position: map.getCenter(),
        draggable: true
    });

    google.maps.event.addListener(marker, 'dragend', function() {
        point = marker.getPosition();
        map.panTo(point);
        showCoords(point);
    });

    google.maps.event.addListener(map, 'dragend', function() {
        point = marker.getPosition();
        marker.setPosition(map.getCenter());
        showCoords(point);
    });

} /* END initialize */

function codeAddress(){
    var address = document.getElementById('address').value;
    geocoder.geocode({'address': address}, function(results, status){
        if(status == google.maps.GeocoderStatus.OK){
            map.setCenter(results[0].geometry.location);
            map.setZoom(15);
            marker.setPosition(map.getCenter());
            showCoords();
        }else{
            alert('Geocode was not successful for the following reason: ' + status)
        }
    });
}

function showCoords(point){
    point = marker.getPosition();
    document.getElementById("lat").value = point.lat().toFixed(5);
    document.getElementById("lng").value = point.lng().toFixed(5);
}

The main change I would make is to reduce the responsibility of codeAddress() to do just the geocoding.  If it successfully looks up the address, then it can pass the location to a callback.

I don't have enough context to tell whether geocoder, map, and marker are global variables, or whether all of this code lies inside some other scope.  Out of habit, I've converted them into local variables.  You decide whether that's right for you.  The point variables in your drag handlers should definitely be made local, though!

function initialize() {
    var geocoder = new google.maps.Geocoder();
    var map = new google.maps.Map(document.getElementById('map-canvas'));

    var marker = new google.maps.Marker({
        map: map,
        position: map.getCenter(),
        draggable: true
    });

    google.maps.event.addListener(marker, 'dragend', function() {
        var point = marker.getPosition();
        map.panTo(point);
        showCoords(point);
    });

    google.maps.event.addListener(map, 'dragend', function() {
        var point = marker.getPosition();
        marker.setPosition(map.getCenter());
        showCoords(point);
    });

    var address = document.getElementById('address').value;

    codeAddress(geocoder, address, function(location) {
        map.setCenter(results[0].geometry.location);
        map.setZoom(15);
        marker.setPosition(map.getCenter());
        showCoords();
    });
}

function codeAddress(geocoder, address, callback) {
    geocoder.geocode({'address': address}, function(results, status) {
        if (status == google.maps.GeocoderStatus.OK) {
            callback(results[0].geometry.location);
        } else {
            alert('Geocode was not successful for the following reason: ' + status)
        }
    });
}


Create a separate JSON + syntax My code already works perfectly. Here is the JS file:

var movieData = {
   count: 6,
   movies: [{
       id: 1,
       title: "Lorem Ipsum",
       thumb: "assets/img/placehold.png"
     },
     {
       id: 2,
       title: "Lorem Ipsum",
       thumb: "assets/img/placehold.png"
     },
     {
       id: 3,
       title: "Lorem Ipsum",
       thumb: "assets/img/placehold.png"
     },
     {
      id: 4,
      title: "Lorem Ipsum",
      thumb: "assets/img/placehold.png"
    },
    {
      id: 5,
      title: "Lorem Ipsum",
      thumb: "assets/img/placehold.png"
    },
    {
      id: 6,
      title: "Lorem Ipsum",
      thumb: "assets/img/placehold.png"
    },
   ]
 };

 $(document).ready(function() {

    if (typeof movieData === 'object' && typeof movieData !== null) {
      // List the movies
      for (var i in movieData.movies) {
         var movie = movieData.movies[i];
         var movieDiv =

         '
' +
            '' +
               '' +
               '

' + movie.title + '' +
            '' +
         '
';

         $('#films').append(movieDiv);
      }
    }

 });

I actually have everything into one file. As a code review I am asking your help if you have any idea about how to improve the code.

Knowing that as an improvement I already though about making this code into 2 different files (one JS and one JSON) but I would like to know your point of view.

I would recommend removing count if all it does is represent the number of items in movies. Otherwise, you'll have 2 sources of truth in the data (count and the length of movies) and code that takes sides down the line (one that believes in count and one that believes in the length of movies). So as much as possible, avoid duplicating data.

Now it's not always feasible to recompute data all the time (i.e. if you had to filter thousands of rows to count "selected" items). This is one case where I'd put a fixed number instead of recomputing it from some property. Just make sure it's documented somewhere.

for (var i in movieData.movies) {

Use regular for loops when looping through arrays. The problem with for-in is that it runs through all enumerable properties, indices and other enumerable properties. You might be getting more than what's just in the array. An even better suggestion is to use array.forEach.

$('#films').append(movieDiv);

The problem with this approach is that you're doing a jquery.append on every item. That means the browser has to re-render every time you add an item. What I suggest is to create a DOM element in memory and append to that in your loop. Then, at the very end, slap the contents of that DOM element onto the page once. This way, the browser only ever renders once.

// Creates a
 that's in memory, not yet appended to the DOM.
const replacementFilms = $('
', { id: '#films' })

movieData.movies.forEach(movie => {
  var movieDiv = '...the markup...'

  // Appends to our
 that's "in memory".
  replacementFilms.append(movieDiv)
})

// Replace the #films in the DOM with the one in memory in one go.
$('#films').replaceWith(replacementFilms)

In this example, I replaced the existing #films in the DOM. But you could also create a dummy element, append the items to it, then append its contents to a container element in the DOM. The operation doesn't have to be a total replace. But the idea is that you only ever touch the DOM once.

     '
' +
        '' +
           '' +
           '

' + movie.title + '' +
        '' +
     '
';

See Template Literals for multi-line strings and string interpolation.


Refactored to use polymorphism in place of case statements The purpose of the below code is to return different types of job objects based on an argument named "jobType."

The original code for the below methods was implemented as a switch statement. I refactored the code removing the switch statement, and in place creating different classes representing different types of jobs.

I would like feedback on my below implementation. The code works, and is released in production, but perhaps there is a better approach.

public bool RunNextJob()
    {
        //  Return true if we found a job to run
        DeferredJob dJob = _jobRepository.GetNextJob();
        SessionProvider sessionProvider = ObjectFactory.GetInstance<SessionProvider>();

        if(dJob != null)
        {
            Logger.Info(string.Format("Running job type: {0} Id: {1}", dJob.JobType, dJob.Id));
            dJob.ProcessedDate = DateTime.Now;
            _jobRepository.Update(dJob);
            sessionProvider.Flush();

            var jobType = new JobType();
            var job = jobType.GetJobType(dJob.JobType);

            if (job != null)
                job.RunJob(dJob);
        }

public class JobType : IJobType
{
    public IJob GetJobType(string jobTypeCode)
    {
        if(jobTypeCode == JobTypeToProcess.Notify.ToString())
            return new NotifyJob();

        if (jobTypeCode == JobTypeToProcess.ConflictedAppointmentMessageJobType.ToString())
            return new ConflictedAppointmentMessageJob();

        if (jobTypeCode == JobTypeToProcess.UnifiedExport.ToString())
            return new UnifiedExportReportJob();

        return jobTypeCode == JobTypeToProcess.FTPJobType.ToString() ? new FTPJob() : null;
    }
}

public class JobType : IJobType
{
    public IJob GetJobType(string jobTypeCode)
    {
        if(jobTypeCode == JobTypeToProcess.Notify.ToString())
            return new NotifyJob();

        if (jobTypeCode == JobTypeToProcess.ConflictedAppointmentMessageJobType.ToString())
            return new ConflictedAppointmentMessageJob();

        if (jobTypeCode == JobTypeToProcess.UnifiedExport.ToString())
            return new UnifiedExportReportJob();

        return jobTypeCode == JobTypeToProcess.FTPJobType.ToString() ? new FTPJob() : null;
    }
}

This seems to be some kind of a factory. So I would rename JobType to JobTypeFactory and IJobType to IJobTypeFactory having the GetJobType() method changed to Create().

instead of using the string representation of the enum you should use Enum<T>.TryParse()
I don't see any advantage on using multiple if statements over a switch..case.
Although braces {} are optional you should use them always to make your code less error prone.

Putting the mentioned points together will lead to

public class JobTypeFactory : IJobTypeFactory
{
    public IJob Create(string jobTypeCode)
    {
        JobTypeToProcess jobType = JobTypeToProcess.None;
        if(!Enum.TryParse<JobTypeToProcess>(jobTypeCode, out jobType)) { return null; }

        switch(jobType)
        {
            case JobTypeToProcess.Notify:
                return new NotifyJob();

            case JobTypeToProcess.ConflictedAppointmentMessageJobType:
                return new ConflictedAppointmentMessageJob();

            case JobTypeToProcess.UnifiedExport:
                return new UnifiedExportReportJob();

            case JobTypeToProcess.FTPJobType:
                return new FTPJob();

            default:
                return null;

        }
    }
}

which is a little bit longer than the original, but it is more readable IMO.

public bool RunNextJob()
{
    //  Return true if we found a job to run
    DeferredJob dJob = _jobRepository.GetNextJob();
    SessionProvider sessionProvider = ObjectFactory.GetInstance<SessionProvider>();

    if(dJob != null)
    {
        Logger.Info(string.Format("Running job type: {0} Id: {1}", dJob.JobType, dJob.Id));
        dJob.ProcessedDate = DateTime.Now;
        _jobRepository.Update(dJob);
        sessionProvider.Flush();

        var jobType = new JobType();
        var job = jobType.GetJobType(dJob.JobType);

        if (job != null)
            job.RunJob(dJob);

        return true;
    }

    return false;
}

By adding a guard clause you could remove some horizontal spacing,
Its a little bit strange setting the ProcessedDate before the job had been processed.
What is the sense of getting the SessionProvider if maybe the job is null ?
for the case you are running C# 6.0 you could use string interpolation instead of string.Format().

Leaving aside the setting of ProcessedDate because I don't know if that would cause sideeffects but including the changes from above, the mentioned points would lead to

public bool RunNextJob()
{
    //  Return true if we found a job to run
    DeferredJob dJob = _jobRepository.GetNextJob();

    if (dJob == null) { return false; }

    SessionProvider sessionProvider = ObjectFactory.GetInstance<SessionProvider>();

    Logger.Info($"Running job type: {dJob.JobType} Id: {dJob.Id}"); // C# 6.0 only

    dJob.ProcessedDate = DateTime.Now;
    _jobRepository.Update(dJob);
    sessionProvider.Flush();

    var jobTypeFactory = new JobTypeFactory();
    var job = jobTypeFactory.Create(dJob.JobType);

    if (job == null) { return false; }

    job.RunJob(dJob);

    return true;

}


Simple ASP.NET C# class design Here's a class I'm designing:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Data;
using System.Data.SqlClient;

namespace Artworking.classes
{

    // A group permission level
    public class PermissionGroup
    {
        public int ID;                  // Unique ID for this group
        public bool hasFullControl;     // Does this group have complete control
        public user creator;            // Who created this group
        public string groupName;        // Reference name for group

        // Constructor for when a permission group ID is passed
        public PermissionGroup(SqlConnection cn, int ID)
        {
            using (SqlCommand cmd = new SqlCommand("SELECT creatorUserID, group_name, fullControl FROM tblATPermissionGroups WHERE ID = " + ID, cn))
            {
                SqlDataReader rdr = cmd.ExecuteReader();
                if (rdr.Read())
                {
                    this.creator.ID = int.Parse(rdr["creatorUserID"].ToString());
                    this.groupName = rdr["group_name"].ToString();
                    this.hasFullControl = bool.Parse(rdr["fullControl"].ToString());
                }
                rdr.Close();
            }
        }
    }

}

Am I on the right track?  Please note I'm not using the built in authentication as it needs to be backwards compatible for an old system. What I'm just checking is that I'm handling these classes properly and loading the data in correctly etc.  As I understand it I'm meant to put my SQL commands in the classes and away from the actual pages right?

It is generally not a good idea to do this type of processing in the .ctor.  Move it to a Load (or some such name) method.  This however means your object is not really loaded after instantiated which is another issue.  As such, I would recommend separating the entity information (HasFullControl, Creator, GroupName, etc) type from the type that loads the information from the database.  You could change PermissionGroup type to just have the data properties and then move the loading logic to a data layer that is responsible for creating the instance a loading the data.

Concatenating text to the end of a sql string leaves you open to Sql Injection attacks (though not in this specific case).  Use a parameter instead:

using (SqlCommand cmd = new SqlCommand("SELECT creatorUserID, group_name,
    fullControl FROM tblATPermissionGroups WHERE ID = @id", cn))
{
    cmd.Parameters.AddWithValue("@id", id);
    ...
}

You need to wrap the SqlDataReader in a using block.

Use public readonly properties rather than public fields.

Consider using a IDbConnection rather than SqlConnection.  Doing so will allow you to more easily mock the data call when testing this method and makes it easier to support other RDBMS's should the need arise.

Throw helpful exceptions if something goes wrong when populating the data fields.

Follow the .Net Framework naming guidelines


Compare performance of Concurrent Dictionary with Dictionary+Locks I wanted to measure the performance of Concurrent Dictionary vs Dictionary+Locks in a multithreaded environment. So I created my own SyncDict class of type<int,int[]>. Whenever there is a key match, it adds the int[] array value to itself, it also locks the whole dictionary with ReaderWriterLockSlim while updating the value.

I replicated the whole code through Concurrent Dictionary and I am using AddOrUpdate() method.
Whole console app code can be found here https://dotnetfiddle.net/1kFbGy Just copy paste the code in console app to run. It will not run fiddle

After running both codes with the same inputs I see a considerable amount of difference in running time. For example for one particular run on my machine Concurrent dictionary took 4.5 seconds vs SyncDict took less than 1 second.

I would like to know any thoughts / suggestions explaining the above running time. Is there anything wrong am I doing here?

 class SyncDict<TKey>
    {
        private ReaderWriterLockSlim cacheLock;
        private Dictionary<TKey, int[]> dictionary;
        public SyncDict()
        {
            cacheLock = new ReaderWriterLockSlim();
            dictionary = new Dictionary<TKey, int[]>();
        }

        public Dictionary<TKey, int[]> Dictionary
        {
            get { return dictionary; }
        }

        public int[] Read(TKey key)
        {
            cacheLock.EnterReadLock();
            try
            {
                return dictionary[key];
            }
            finally
            {
                cacheLock.ExitReadLock();
            }
        }

        public void Add(TKey key, int[] value)
        {
            cacheLock.EnterWriteLock();
            try
            {
                dictionary.Add(key, value);
            }
            finally
            {
                cacheLock.ExitWriteLock();
            }
        }

        public AddOrUpdateStatus AddOrUpdate(TKey key, int[] value)
        {
            cacheLock.EnterUpgradeableReadLock();
            try
            {
                int[] result = null;
                if (dictionary.TryGetValue(key, out result))
                {
                    if (result == value)
                        return AddOrUpdateStatus.Unchanged;
                    else
                    {
                        cacheLock.EnterWriteLock();
                        try
                        {
                            Parallel.For(0, value.Length,
                            (i, state) =>
                            {
                                result[i] = result[i] + value[i];
                            });
                        }
                        finally
                        {
                            cacheLock.ExitWriteLock();
                        }
                        return AddOrUpdateStatus.Updated;
                    }
                }
                else
                {
                    Add(key, value);
                    return AddOrUpdateStatus.Added;
                }
            }
            finally
            {
                cacheLock.ExitUpgradeableReadLock();
            }
        }

        public void Delete(TKey key)
        {
            cacheLock.EnterWriteLock();
            try
            {
                dictionary.Remove(key);
            }
            finally
            {
                cacheLock.ExitWriteLock();
            }
        }

        public enum AddOrUpdateStatus
        {
            Added,
            Updated,
            Unchanged
        };
    }

Comparison

A comparison of performance of 2 classes only makes sense if both adhere to the same specification. Does your class do what a ConcurrentDictionary does?

Review

Why would you allow access to the underlying dictionary? If you must allow it, return a IReadOnlyDictionary.
Checking arguments before taking a lock prevents unnecessary locks on bad input.
AddOrUpdate does not work; have you checked  if (result == value)?

int[] a = new int[]{ 0, 1 };
int[] b = new int[]{ 0, 1 };
Console.WriteLine(a == b);  // False

Or is this as designed?

What is the purpose of this? What if both arrays have different size?

 Parallel.For(0, value.Length,
 (i, state) =>
 {
     result[i] = result[i] + value[i];
 });


Code feedback on running app I have made an Android app called RunTracker - https://github.com/michaelnares/runtracker/tree/master/app.  The intended functionality is that the app saves runs to the local filesystem, shows your current location and current run, and has a clickable list of runs.  I feel that the existing code can be improved, but I'm not sure how.  For example, the code for the MainActivity is quite verbose:

package com.michaelnares.runtracker.activities;

import android.arch.lifecycle.ViewModelProviders;
import android.content.Intent;
import android.support.v7.app.AppCompatActivity;
import android.os.Bundle;
import android.util.Log;
import android.view.View;
import android.view.WindowManager;
import android.widget.Button;
import android.widget.CompoundButton;
import android.widget.Switch;

import com.michaelnares.runtracker.R;
import com.michaelnares.runtracker.data.RunPoint;
import com.michaelnares.runtracker.utils.PermissionsHandler;
import com.michaelnares.runtracker.utils.RunsViewModel;
import com.michaelnares.runtracker.utils.Utils;

import org.json.JSONException;

import java.io.IOException;
import java.util.ArrayList;

public class MainActivity extends AppCompatActivity {

    private ArrayList<RunPoint> runPointsList = new ArrayList<>();
    private RunsViewModel runsViewModel;
    private AppCompatActivity activity = this;
    private PermissionsHandler permissionsHandler;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        permissionsHandler = new PermissionsHandler(this, MainActivity.this);
        setContentView(R.layout.activity_main);
        permissionsHandler.checkForPermissions();
        runsViewModel = ViewModelProviders.of(this).get(RunsViewModel.class);
        runsViewModel.setActivity(this);
        runsViewModel.setContext(getApplicationContext());

        final Button goToListButton = findViewById(R.id.goToList);
        goToListButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                final Intent intent = new Intent(MainActivity.this, RunsListActivity.class);
                startActivity(intent);
            }
        }); // The setOnClickListener() block ends here.

        final Switch locationSwitch = findViewById(R.id.locationSwitch);
        locationSwitch.setChecked(runsViewModel.canTrackLocation());
        locationSwitch.setOnCheckedChangeListener(new CompoundButton.OnCheckedChangeListener() {
            @Override
            public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
                runsViewModel.setCanTrackLocation(isChecked);
                if (isChecked && permissionsHandler.allConditionsMet()) {
                    permissionsHandler.startTrackingRuns();
                } else { // So save the current run, if the user switches the toggle off.
                    saveCurrentRun();
                }
            }
        });
        getWindow().addFlags(WindowManager.LayoutParams.FLAG_KEEP_SCREEN_ON); // Keeps the screen on always, when the app is running.
    } // The onCreate() method ends here

    private void saveCurrentRun() {

        if (runsViewModel == null) {
            runsViewModel = ViewModelProviders.of(this).get(RunsViewModel.class);
        }
        try {
            runsViewModel.saveCurrentRun(); // Saving them to external storage.
        } catch (JSONException e) {
            Log.e(Utils.getPackageName(MainActivity.this), "There was a JSONException whilst trying to save the journey points.");
            e.printStackTrace();
        } catch (IOException e) {
            Log.e(Utils.getPackageName(MainActivity.this), "There was an IOException whilst trying to save the journey points.");
            e.printStackTrace();
        }
    }
} // The main class ends here.

Can I rewrite the code to achieve better separation of concerns, i.e. less code in the MainActivity.  How can the code be improved elsewhere?

I don't know anything about Android programming, so bear with me.

Unused variable

private AppCompatActivity activity = this;

This is unused - and even if it were used, you could just replace all references of it with this.

runPointsList is similarly unused.

Outer class this references

This code:

        @Override
        public void onClick(View v) {
            final Intent intent = new Intent(MainActivity.this, RunsListActivity.class);
            startActivity(intent);
        }

is justified in referencing MainActivity.this because it's an inner class. This code:

    permissionsHandler = new PermissionsHandler(this, MainActivity.this);

is not. You should be able to drop MainActivity. .

Multiple exceptions

This:

    } catch (JSONException e) {
        Log.e(Utils.getPackageName(MainActivity.this), "There was a JSONException whilst trying to save the journey points.");
        e.printStackTrace();
    } catch (IOException e) {
        Log.e(Utils.getPackageName(MainActivity.this), "There was an IOException whilst trying to save the journey points.");
        e.printStackTrace();
    }

should be able to have its two handlers collapsed to one with catch (JSONException|IOException e) syntax, since they do almost the same thing.


Lambda expression to check even numbers I wrote this code to check the even numbers from the List. After finding even number I twice the even numbers with the help of Map and then find the sum of each even number after making it double by use of the reduce method.

import java.util.Arrays;
import java.util.List;

public static void main(String[] args) {
List<Integer> numbers = Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9);

System.out.println(
numbers.stream()
        .filter(e -> e % 2 == 0)
        .map( e -> e * 2)
        .reduce(0, Integer::sum));
}

Have you come across the IntStream?  I'd use that for numerical operations rather than the boxed version Stream<Integer>.


Course Statistics Using Arrays and Files I wanted to get some feedback here on my solution, because the course I am using is not allowing me to submit any answers.

  Objectives:

  Write a Java program that:

  Create a grades.txt file with some grades inputted in
  Reads these grades from the file and stores it into an ArrayList
  After storing all elements into the ArrayList return the max, min, and average grade of the list
  Return an Arraylist without any duplicate grades. All duplicated grades must be removed

import java.io.*;
import java.lang.reflect.Array;
import java.util.*;

public class Main {

    public static void main(String[] args) throws IOException {
       File file = new File("grades.txt");

      PrintWriter output = new PrintWriter(file);
      output.println("12.5");
      output.println("19.75");
      output.println("11.25");
      output.println("10");
      output.println("15");
      output.println("13.25");
      output.println("14");
      output.println("9");
      output.println("10");
      output.println("19.75");
      output.close();

      // Reading from the file //

        ArrayList<Double> gradesString = new ArrayList<Double>();

        Scanner input = new Scanner(file);

        while(input.hasNext())
        {
            String line = input.nextLine();
            gradesString.add(Double.parseDouble(line));
        }

        double result = 0;

        for(Double i : gradesString)
        {
            result += i;
        }

        double mean = result / gradesString.size();

        LinkedHashSet<Double> hashSet = new LinkedHashSet<>(gradesString);

        ArrayList<Double> gradesWithoutDuplicates = new ArrayList<Double>(hashSet);

        System.out.println("The grades are: " + gradesString);
        System.out.println("The highest grade is: " + Collections.max(gradesString));
        System.out.println("The lowest grade is: " + Collections.min(gradesString));
        System.out.println("The average is: " + mean);
        System.out.println("The grades list without duplicates is: " + gradesWithoutDuplicates);

    }

}

Output:

The grades are: [12.5, 19.75, 11.25, 10.0, 15.0, 13.25, 14.0, 9.0, 10.0, 19.75]
The highest grade is: 19.75
The lowest grade is: 9.0
The average is: 13.45
The grades list without duplicates is: [12.5, 19.75, 11.25, 10.0, 15.0, 13.25, 14.0, 9.0]

Exception handling

It is not customary to let the main method throw an exception. At the very least you can catch the base Exception class and use printStackTrace() to print the error message and related debug info.

IO handling

This program only deals with one file.  However, it is good practice to remember to close every external resource.  That includes files, database connections (and other related resources, like prepared statements and result sets), network sockets, etc. not closing resources may result in resource leaks that can escalate to JVM crash. Java 7 introduced the try-with-resources construct that allows the compiler to automatically handle resource closure as well as error handling.
Java 7 also introduced the "new io" java.nio package that improves IO handling in Java.   the Files.readAllLines() is a convenient method that reads whole file into List of Strings (and also internally handles the external resource). (alas, no equivalent writeAllLines())

Naming conventions

gradesString is a List of Double items. the name is very confusing.


Simple Quadratic Equation Solver Still pretty new to coding. I posted that Twin Primes Finder the other day and you guys were a great help so thanks. Here's my quadratic equation solver:

import math

#variables for initial equation
a = int(raw_input("Enter a:"))
b = int(raw_input("Enter b:"))
c = int(raw_input("Enter c:"))

#variables for quad. equation
negative_b = -b
b_squared = b**2
four_a_c = 4*a*c
determinant = b_squared - four_a_c

#prints initial equation
equation = [str(a)+"x^2", str(b)+"x", str(c)]

print "Solve: " + " + ".join(equation)

if determinant < 0:
    print "No real solutions."
elif a == 0:
    y_int = float(-c)/b
    print "x =", y_int
    print "But this is a straight line."
else:
    root_determinant = math.sqrt(determinant)
    two_a = 2*a
    numerator_one = negative_b - root_determinant
    answer_one = numerator_one/two_a
    if determinant == 0:
      print "x =", answer_one
    else:
      numerator_two = negative_b + root_determinant
      answer_two = numerator_two/two_a
      print "x =", answer_one, "or", answer_two

I wold separate the concerns somewhat and put them into functions.

One major concern is actually solving the quadratic equation. Another is the fancy input/output around it (if you want to be pedantic, that is actually two concerns).

The solving function, which I would call solve_quadratic returns a list of the solutions. This list can be empty (if there are no resolutions) or have multiple entries. We can use this fact to just append a solution if there are multiple.

I inlined some of the computations, if they were only used once.

import math

def solve_quadratic(a, b, c):
    """
    Given three real coefficients,
    returns the (real) roots of the second degree polynomial
    """
    negative_b = -b
    determinant = b**2 - 4*a*c

    if determinant < 0:
        return []
    elif a == 0:
        return [float(-c)/b]
    else:
        root_determinant = math.sqrt(determinant)
        two_a = 2*a
        answers = [(negative_b - root_determinant)/two_a]
        if determinant:
            answers.append((negative_b + root_determinant)/two_a)
        return answers

Now, the input part and the printing which serves as interpretation. For the initial printing I would use str.format, instead of first building a list and then joining it. It is slightly more readable IMO.

Finally, I would use if __name__ == "__main__": to be able to import these functions in another script without calling the input function.

def fancy_quadratic_solver():
    a = int(raw_input("Enter a:"))
    b = int(raw_input("Enter b:"))
    c = int(raw_input("Enter c:"))
    print "Solve: {}x^2 + {}x + {}".format(a, b, c)

    x = solve_quadratic(a, b, c)
    if not x:
        print "No real solutions."
    else:
        print "x =", " or ".join(map(str, x))
        if a == 0:
            print "But this is a straight line."

if __name__ == "__main__":
    fancy_quadratic_solver()


Sum over selected numpy.ndarray column and write to a file I am trying to write the sum of selected NumPy array column and write to a file.
The following code snippet is working fine:

for i in range(int(ldata[2])):
    s_u.append(dos2[i][1])
    s_d.append(dos2[i][2])
    p_u.append(np.sum(dos2[i,[3,5,7]]))
    p_d.append(np.sum(dos2[i,[4,6,8]]))
    d_u.append(np.sum(dos2[i,[9,11,13,15,17]]))
    d_d.append(np.sum(dos2[i,[10,12,14,16,18]]))
with open("dos.dat","w") as ados:
    for i in range(int(ldata[2])):
        ados.write(str(atom[0,i,0]-float(ldata[3]))+" ")
        ados.write(str(s_u[i])+" ")
        ados.write(str(s_d[i])+" ")
        ados.write(str(p_u[i])+" ")
        ados.write(str(p_d[i])+" ")
        ados.write(str(d_u[i])+" ")
        ados.write(str(d_d[i])+" ")
        ados.write("\n")
print(type(dos2))   #<class 'numpy.ndarray'>
print(type(s_u))    #<class 'list'>

But I am feeling like I am overdoing things. Also, I am not checking how many columns (j) there are in dos2[i,[j]] (which may vary).

Can I get some help to make it better?

I will assume that all of the ?_? lists are empty when you start.  Here are my suggestions:

Rather than using lists, I would just make the ?_? variables numpy arrays.  You can slice the values you want from dos2 using syntax like s_u = dos2[:int(ldata[2]), [3,5,7]].
Your indexing is really slices with a step size of two.  So [3, 5, 7] is 3:9:2, [4,6,8] is 4:10:2, [9,11,13,15,17] is 9:19:2, and [10,12,14,16,18] is 10:20:2.  Numpy is much faster with these sorts of slices rather than using the direct indexing you are using since with these slices numpy can avoid making a copy.
Since everything is done in steps of 2, I think it would make it even easier to first reshape dos2 to a 3D array, where the last dimension determines whether it is a ?_u or ?_d variables.  Then you can use unpacking to put the columns in the right place.
You can use np.column_stack to combine all of your 1D arrays into one big 2D array.  This can then be written in one step using np.savetxt.
Better yet, you can use a slice for the last index of atom to end up with all 2D arrays, then just hstack them to get one big array.  This avoids having to unpack at all.
I prefer the arr.sum() syntax of np.sum(arr) since I think it makes the flow easier to follow for people who read left-to-right.

So here is how I would implement what you are doing (with comments and dummy data):

# Dummy test data
dos2 = np.random.random((1000, 19))
atom = np.random.random((2, 1000, 2))
ldata = [None, None, '100', '1.1']

# We re-use this value several times, so put it in a variable
maxind = int(ldata[2])

# Extract the part of dos2 we want, and reshape it so the last index
# determines whether it is a ?_u (index==0) or ?_d (index==1) value
dos2_r = dos2[:maxind, 1:19].reshape(-1, 9, 2)

# Extract the part of atom we want
# We use slicing instead of indexing for the last index to keep it 2D
atom_p = atom[0,:maxind,:1]-float(ldata[3])

# Slice out the parts of dos2 we want
s_ud = dos2_r[:, 0, :]
p_ud = dos2_r[:, 1:4, :].sum(axis=1)
d_ud = dos2_r[:, 4:, :].sum(axis=1)

# Combine the arrays into one big array
cols = np.hstack([atom_p, s_ud, p_ud, d_ud])

# Write to a file
np.savetxt('dos.dat', cols, delimiter=' ')

Edit:

OP asked my to explain how the slicing is working.  I think the easiest way to explain is to walk through a toy example.  Lets start with a 2D array. That I think makes it easy to follow what is going on:

>>> dos2 = np.vstack([np.arange(12)+100*i for i in range(20)]).T
>>> print(dos2)
[[   0  100  200  300  400  500  600  700  800  900 1000 1100 1200 1300 1400 1500 1600 1700 1800 1900]
 [   1  101  201  301  401  501  601  701  801  901 1001 1101 1201 1301 1401 1501 1601 1701 1801 1901]
 [   2  102  202  302  402  502  602  702  802  902 1002 1102 1202 1302 1402 1502 1602 1702 1802 1902]
 [   3  103  203  303  403  503  603  703  803  903 1003 1103 1203 1303 1403 1503 1603 1703 1803 1903]
 [   4  104  204  304  404  504  604  704  804  904 1004 1104 1204 1304 1404 1504 1604 1704 1804 1904]
 [   5  105  205  305  405  505  605  705  805  905 1005 1105 1205 1305 1405 1505 1605 1705 1805 1905]
 [   6  106  206  306  406  506  606  706  806  906 1006 1106 1206 1306 1406 1506 1606 1706 1806 1906]
 [   7  107  207  307  407  507  607  707  807  907 1007 1107 1207 1307 1407 1507 1607 1707 1807 1907]
 [   8  108  208  308  408  508  608  708  808  908 1008 1108 1208 1308 1408 1508 1608 1708 1808 1908]
 [   9  109  209  309  409  509  609  709  809  909 1009 1109 1209 1309 1409 1509 1609 1709 1809 1909]
 [  10  110  210  310  410  510  610  710  810  910 1010 1110 1210 1310 1410 1510 1610 1710 1810 1910]
 [  11  111  211  311  411  511  611  711  811  911 1011 1111 1211 1311 1411 1511 1611 1711 1811 1911]]

This has 12 values in the first dimension and 20 in the second, so it has some extra data rows and columns.  I chose this particular array since, as you can see, for value xyy or xxyy, the x or xx value is the column number and the yy value is the row number.  This makes it easy to keep track of what the manipulations are doing.

Let's make maxind = 10 for simplicity.  So the first thing I do is slice out the data you don't want.  You never use the first column, and you never use past the 18th column.  Also, you never use past maxind rows.  So I throw out everything besides that.  I will call the result dos2_p.  Note that with slicing, for arr[0:n] the 0 is implied so it can be written as arr[:n], and for arr[n:len(arr)] the len(arr) is implied so it can be written as arr[n:].  arr[:] means "everthing", and arr[:, n] means "everything from dimension 0 and the nth index of dimension 1".  arr[..., n] means "the nth index of the last dimension and everything from the rest of the dimensions, however many that might be"

>>> maxind = 10
>>> dos2_p = dos2[:maxind, 1:19]
>>> print(dos2_p)
[[ 100  200  300  400  500  600  700  800  900 1000 1100 1200 1300 1400 1500 1600 1700 1800]
 [ 101  201  301  401  501  601  701  801  901 1001 1101 1201 1301 1401 1501 1601 1701 1801]
 [ 102  202  302  402  502  602  702  802  902 1002 1102 1202 1302 1402 1502 1602 1702 1802]
 [ 103  203  303  403  503  603  703  803  903 1003 1103 1203 1303 1403 1503 1603 1703 1803]
 [ 104  204  304  404  504  604  704  804  904 1004 1104 1204 1304 1404 1504 1604 1704 1804]
 [ 105  205  305  405  505  605  705  805  905 1005 1105 1205 1305 1405 1505 1605 1705 1805]
 [ 106  206  306  406  506  606  706  806  906 1006 1106 1206 1306 1406 1506 1606 1706 1806]
 [ 107  207  307  407  507  607  707  807  907 1007 1107 1207 1307 1407 1507 1607 1707 1807]
 [ 108  208  308  408  508  608  708  808  908 1008 1108 1208 1308 1408 1508 1608 1708 1808]
 [ 109  209  309  409  509  609  709  809  909 1009 1109 1209 1309 1409 1509 1609 1709 1809]]

As you can see, each rows start with the 1?? value and end with the 18?? value, which means it starts with columns 1 and ends with column 18, and the maximum ?yy value is 09, which means it ends with row 9.

Now I want to make it so the even rows are in one dimension, and the odd rows are in another.  I can do that with reshape.  It can take some trial-and-error to get the reshape to work the way you want.  I don't actually have a good conceptual way to do this, I just create a dummy example like this and try different things until I get the layout I want.  In this case, we want to set the last dimension to have a shape of 2, the first to have the same shape as currently (maxind), and we can let numpy figure out the second dimension (although we know it should be 9).

>>> dos2_r = dos2_p.reshape(maxind, -1, 2)
>>> print(dos2_r[..., 0])
[[ 100  300  500  700  900 1100 1300 1500 1700]
 [ 101  301  501  701  901 1101 1301 1501 1701]
 [ 102  302  502  702  902 1102 1302 1502 1702]
 [ 103  303  503  703  903 1103 1303 1503 1703]
 [ 104  304  504  704  904 1104 1304 1504 1704]
 [ 105  305  505  705  905 1105 1305 1505 1705]
 [ 106  306  506  706  906 1106 1306 1506 1706]
 [ 107  307  507  707  907 1107 1307 1507 1707]
 [ 108  308  508  708  908 1108 1308 1508 1708]
 [ 109  309  509  709  909 1109 1309 1509 1709]]
>>> print(dos2_r[..., 1])
[[ 200  400  600  800 1000 1200 1400 1600 1800]
 [ 201  401  601  801 1001 1201 1401 1601 1801]
 [ 202  402  602  802 1002 1202 1402 1602 1802]
 [ 203  403  603  803 1003 1203 1403 1603 1803]
 [ 204  404  604  804 1004 1204 1404 1604 1804]
 [ 205  405  605  805 1005 1205 1405 1605 1805]
 [ 206  406  606  806 1006 1206 1406 1606 1806]
 [ 207  407  607  807 1007 1207 1407 1607 1807]
 [ 208  408  608  808 1008 1208 1408 1608 1808]
 [ 209  409  609  809 1009 1209 1409 1609 1809]]

See that index 0 of the last dimension has all the odd columns, while dimension 1 has all the even columns.

Now let's do a min in this case instead of a sum.  The semantics are the same, but it is easier to follow in my opinion.  We will take the min of dimension 1, which is what used to be columns, but now is the columns split into even and odd:

>>> dos2_m = dos2_r.min(axis=1)
>>> print(dos2_m)
[[100 200]
 [101 201]
 [102 202]
 [103 203]
 [104 204]
 [105 205]
 [106 206]
 [107 207]
 [108 208]
 [109 209]]

As you can see, it preserves the row number, and it preserves whether the column is even or odd, but it collapses across even columns and odd columns.

Now let's split it the way it is split in your code:

>>> ms_ud = dos2_r[:, 0, :]
>>> mp_ud = dos2_r[:, 1:4, :].min(axis=1)
>>> md_ud = dos2_r[:, 4:, :].min(axis=1)
>>> print(ms_ud)
[[100 200]
 [101 201]
 [102 202]
 [103 203]
 [104 204]
 [105 205]
 [106 206]
 [107 207]
 [108 208]
 [109 209]]
>>> print(mp_ud)
[[300 400]
 [301 401]
 [302 402]
 [303 403]
 [304 404]
 [305 405]
 [306 406]
 [307 407]
 [308 408]
 [309 409]]
>>> print(md_ud)
[[ 900 1000]
 [ 901 1001]
 [ 902 1002]
 [ 903 1003]
 [ 904 1004]
 [ 905 1005]
 [ 906 1006]
 [ 907 1007]
 [ 908 1008]
 [ 909 1009]]

So you can see it takes the same columns you did in your original, but keeps them in a single 2D array.  And it greatly simplifies the code and speeds up the computation as well.