Get the Key of the highest Value in JavaScript object Given a hashmap (or a "Dictionary") as a JavaScript object, I'd like to get the Key who has the maximum Value - assuming all Values are integers.

In case there's more than one, I don't care which one of them.

Considering

var b = { '1': 9, '2': 7, '3': 7, '4': 9, '5': 3 };

I can extract the desired Key by:

parseInt(_(b).chain().pairs().max(function(p){return p[1];}).value()[0])

which returns 1.

How can this be achieved more elegantly?

I tried with _.invert as well but couldn't make it look better.

I don't know about elegant, but you can omit the initial _(), use + to convert to number instead of parseInt and just pull out the iterator.

var iterator = function(p){return p[1];}
var maxKey = +_.chain(b).pairs().max(iterator).value()[0];
// Removed the | 0 since the keys are integers to begin with

Here's an alternate way to do it using ES5 Array.prototype.reduce and Object.keys. You can also use underscore's _.reduce and _.keys for compatibility. Note that this runs through the entire array, so any duplicates found will use the latest value.

var max = Object.keys(b).reduce(function(max,key){
  return (max === undefined || b[key] > b[max]) ? +key : max;
});

Making it more clear what we're doing, in case you aren't familiar of reduce, is to use Array.prototype.forEach (or _.each):

var max;
Object.keys(b).forEach(function(key){
  max = (b[key] > b[max]) ? +key : max;
});


Checking if an array element exists in a JS object I have a feature in my application that should allow users to select items in a drop down menu and move them up and down. Once they select and click on the button I have to loop over the array of objects and pull only records that were selected in the drop down menu.

Here is an example of my code:

var selectedColumns = ['first','last','city'];

var data = [
   {
      first: "Mike",
      last: "Ross",
      dob: "05/26/1978",
      city: "Washington DC",
      state: "DC",
      zip: 22904
   },
   {
      first: "John",
      last: "Henderson",
      dob: "11/06/1988",
      city: "Iowa City",
      state: "IA",
      zip: 52401
   },
   {
      first: "Nina",
      last: "Barkley",
      dob: "01/16/1968",
      city: "New York",
      state: "NY",
      zip: 11308
   },
   {
      first: "Jessie",
      last: "Kuch",
      dob: "02/02/1956",
      city: "Des Moines",
      state: "IA",
      zip: 55432
   },
   {
      first: "Jenny",
      last: "Terry",
      dob: "012/28/1988",
      city: "Miami",
      state: "FL",
      zip: 83943
   }
];

In the selected column we only have first, last and city. Then I have to loop over data and pull only selected columns. One way to do that is like this:

for(var key in data){
   for(var i=0; i<selectedColumns.length; i++){
      var columnID = String(columns[i]);
      console.log($.trim(data[key][columnID]));
   }
}

While this solution works just fine, I'm wondering if there is better way to avoid the inner loop and improve efficiency? I use jQuery/JavaScript in my project. If anyone knows a better way to approach this problem please let me know.

First of all, don't use for-in on arrays. for-in will loop over all enumerable properties, including non-index properties. Also, for-in order is not guaranteed. If you need a loop to go over array items only and in order, use for/while. Alternatively you can use array.forEach.

There's no way (that I know of) to remove that second loop, since you need to go over each selected property for each person in data. What you could do though is use native array methods. This way, you avoid the extra boilerplate that comes with loops like not going over the actual number of items.

What you really want is array.map. It's primary purpose is to create a new array based on another array, essentially a 1:1 transformation. In your case, you need to create an array that contains the same items but only selected properties.

For the inner loop, I suggest array.reduce. One use case for reduce is to condense an array of values into a single value. In this case, the selected keys into an object only containing the selected keys and their values.

var selectedColumns = ['first', 'last', 'city'];

var data = [{
    first: "Mike",
    last: "Ross",
    dob: "05/26/1978",
    city: "Washington DC",
    state: "DC",
    zip: 22904
  },
  {
    first: "John",
    last: "Henderson",
    dob: "11/06/1988",
    city: "Iowa City",
    state: "IA",
    zip: 52401
  },
  {
    first: "Nina",
    last: "Barkley",
    dob: "01/16/1968",
    city: "New York",
    state: "NY",
    zip: 11308
  },
  {
    first: "Jessie",
    last: "Kuch",
    dob: "02/02/1956",
    city: "Des Moines",
    state: "IA",
    zip: 55432
  },
  {
    first: "Jenny",
    last: "Terry",
    dob: "012/28/1988",
    city: "Miami",
    state: "FL",
    zip: 83943
  }
];

const onlySelected = data.map(person => {
  return selectedColumns.reduce((c, key) => {
    return { ...c, [key]: person[key] }
  }, {})
})

console.log(onlySelected)


Create a text index of files This program creates a text index of 3 different files in Python. While this works correctly, I am still a beginner in Python. I used a list of class objects. Can this code be further optimised?

class ListNode :
    def __init__(self,w,docslist) :

        self.word = w
        self.docslist = []
        self.docslist.append(docslist)

TextIndex = []

def Search(word,TextIndex) :

    for i in TextIndex :
        if i.word == word :
            return TextIndex.index(i)
        else :
            continue

    return -1

def Create(docs) :
    for d in docs :
        words = []
        for line in d :
            line = line.split()
            for word in line :
                if word not in words :
                    words.append(word)

            for word in words :

                node = ListNode(word,d.name)

                j = Search(word,TextIndex)

                if j == -1 :
                    TextIndex.append(node)

                else :
                    TextIndex[j].docslist.append(d.name)

def Display() :
    TextIndex.sort(key=lambda x: x.word, reverse=False)
    for i in TextIndex :
        print (i.word,i.docslist)

def main() :

    try :
        with open(r"C:\Users\Anonymous\Documents\words1.txt") as a, open(r"C:\Users\Anonymous\Documents\words2.txt") as b,       open(r"C:\Users\Anonymous\Documents\words3.txt") as c :
        docs = [a,b,c]
        Create(docs)

        Display()

    except IOError as e :
        print ('Operation Failed: %e' %e.strerror)

if __name__ == '__main__':
    main()

Coding style

Please follow PEP8, the Python coding style guide.
You have really a LOT of coding style violations,
and it's really hard to read your code this way.

In addition, the variable names are extremely poor,
especially a file handle named d,
and a node named i,
but an index named j is also not great.

Choice of data structure

The index is backed by TextIndex, a list,
which is inefficient.
The time complexity of a lookup (the Search function) is \$O(n)\$.
You can do much better by using a dictionary,
mapping words to nodes.
In fact, the Search function would completely disappear,
as you could replace it with a simple condition like word in TextIndex to check if something is in the index or not.

Working with files

Since all 3 input files are processed exactly the same way,
there's no need to process them all in the same with block.
If an error occurs during the processing of one,
the program would exit, so this is fragile,
and at the same time a very rigid setup.
It would be better to have a helper function in charge of opening and processing a file,
and call this function for each file sequentially.

It's also not great to pass a file handle to another function.
It's a lot clearer to work with a file handle in the with block,
read data from it, and then it's ok to pass the read data to another function for further processing.


Equilibrium Index Implementation I have come up with a solution to the equilibrium index problem with help from the answers off here:

val in = Stream(-7, 1, 5, 2, -4, 3, 0)
val sum = in.scan(0)(_ + _)
val hash = (in zip sum) map { case (e, lsum) => 2*lsum + e }
val target = sum.last
hash.zipWithIndex.collect { case (e, i) if e == target => i }

It's unfortunate that I have to traverse through the whole stream again to get target.

Are there any possible improvements?

hash is poorly named, I think, but that's a minor detail.

Your solution is inefficient because you are doing it all with sequences.  Have you considered that zipWithIndex is also duplication?  You've already worked through the input stream once - each time you calculated the current sum, you could (had you wanted) have been aware of your position in the list?  You're duplicating work.

More efficient solution (in that there will only be one traversal) is to fold, not scan and to build a map (for quick extraction of results).

Fold over the input stream, with a seed of (0, 0, Map[Int, List[Int]]).  The first value is an index accumulator, the second is a sum accumulator and the nature of the map will become obvious shortly.  The function you apply to each fold iteration should

Add the current stream item to twice the accumulated sum.
If the resulting number is not a key in the map, insert `list(index-accumulator-value)' into the map with that newly-calculated number as a key
If the resulting number is already a key, add index-accumulator-value to the list stored under that key (important: cons it to the list, do not append).
Increment the index accumulator by one.
Add the current stream item to the accumulated sum.
Return (index-accumulator, sum-accumulator, map)

Once the fold has completed, you will have

The size of the stream (which you don't need)
The sum of the stream (which you do)
A map.

If the map contains a key which is equal to the sum of the stream, the value associated with that key will be a list of all equilibrium indices, sorted in reverse order.  If there is no such key, there were no equilibrium indices.

Minimal traversal
No filtering necessary
Rapid access to final result because Map

Done.

Annoyed that the final result is in reverse order?  Reversing one final list is cheap compared to appending to each list in the map, every time.

Note that you can dispense with having to manage the index-accumulator by folding over in.zipWithIndex


EF6 Code First unit of work pattern with IoC/DI I'm trying to implement the unit of work pattern with dependency injection / inversion of control and entity framework version 6.1.1 Code First, in an asp.net-mvc project.

public interface IGenericRepository<T> : where T : class
{
    IQueryable<T> AsQueryable();

    IEnumerable<T> GetAll();
    IEnumerable<T> Find(Expression<Func<T, bool>> predicate);
    T Single(Expression<Func<T, bool>>  predicate);
    T SingleOrDefault(Expression<Func<T, bool>> predicate);
    T First(Expression<Func<T, bool>> predicate);
    T GetById(int id);

    void Add(T entity);
    void Delete(T entity);
    void Attach(T entity);
}

public interface IUnitOfWork : IDisposable
{
    IGenericRepository<Order> OrderRepository { get; }
    IGenericRepository<Customer> CustomerRepository { get; }
    IGenericRepository<Employee> EmployeeRepository { get; }

    void Commit();
}

public class EfUnitOfWork : DbContext, IUnitOfWork
{
    private readonly EfGenericRepository<Order> _OrderRepo;
    private readonly EfGenericRepository<Customer> _customerRepo;
    private readonly EfGenericRepository<Employee> _employeeRepo;

    public DbSet<Order> Orders { get; set; }
    public DbSet<Customer> Customers { get; set; }
    public DbSet<Employee> Employees { get; set; }

    public EfUnitOfWork()
    {
        _orderRepo = new EfGenericRepository<Order>(Orders);
        _customerRepo = new EfGenericRepository<Customer>(Customers);
        _employeeRepo = new EfGenericRepository<Employee>(Employees);
    }

    #region IUnitOfWork Implementation

    public IGenericRepository<Order> OrderRepository
    {
        get { return _orderRepo; }
    }

    public IGenericRepository<Customer> CustomerRepository
    {
        get { return _customerRepo; }
    }

    public IGenericRepository<Employee> EmployeeRepository
    {
        get { return _employeeRepo; }
    }

    public void Commit()
    {
        this.SaveChanges();
    }

    #endregion
}

public class EfGenericRepository<T> : IGenericRepository<T>
                                      where T : class
{
    private readonly DbSet<T>_dbSet;

    public EfGenericRepository(DbSet<T> dbSet)
    {
        _dbSet = dbSet;
    }

    #region IGenericRepository<T> implementation

    public virtual IQueryable<T> AsQueryable()
    {
        return _dbSet.AsQueryable();
    }

    public IEnumerable<T> GetAll()
    {
        return _dbSet;
    }

    public IEnumerable<T> Find(Expression<Func<T, bool>> predicate)
    {
        return _dbSet.Where(predicate);
    }

    // And so on ...

    #endregion
}

And this is how I'm using it in a controller:

public HomeController(IUnitOfWork unitOfWork) { ..... }

But I've read that EF6 already has Unit of Work out of the box. How would that fit in this picture?

Also, with which dependency injector should I go with?

Unit of work

There are some issues of mechanics here. Normally with a generic repository, you want to be able to extend it, so you'd write:

public interface IOrderRepository : IGenericRepository<Order>
{
    //Some Order-specific queries here
}

But this would mean updating your IUnitOfWork every time too.

You're also having to add lots of annoying boilerplate code to your EfUnitOfWork to make it conform to the interface. The question here has a much nicer way of achieving the same thing.

However, my actual suggestion would be to remove IUnitOfWork altogeter, and simply add a Save (or, if you prefer, Commit) method to your repositories. Then any code which needs data access should be passed the repositories it needs directly, rather than ever being passed the DbContext.

The Generic Repository

I think the first hint that you're putting the cart before the horse here is the name you picked: IGenericRepository. The fact that it's generic absolutely does not need to be in the name. For one thing, it's already implied by the generic parameter. But more importantly, the fact that you're using the generic repository pattern, as opposed to just the repository pattern, is an unimportant detail.

The generic repository is simply a base class for your repositories. The only reason it's there is that there will be some methods you'll want on all of your actual repositories, and you don't want to have to repeat them.

Mat's Mug already gave the reasons in his answer not to expose IQueryable or take Expression or Func arguments in your repository, and I'd strongly echo those.

How to write your repositories

So given that, here's what I think a generic repository should look like to start out:

public interface IRepository<T> { }

Why is it empty? Because you don't know what methods you're going to need yet.

As soon as you need data access in one of your classes, write a repository for it:

public interface IOrderRepository : IRepository<T>
{
    IEnumerable<Order> GetAllPendingOrders(DateTime endDate);

    void Add(Order order);
}

(GetAllPendingOrders and Add being made up example data access methods that you might find yourself needing to use in some service class)

Note how these methods are defined by what the repository's consumer needs, they're not just aimed to exactly match what IDbSet already gives you.

After you have a few repositories, you'll find you have quite a bit of repetition. For example, Add would probably be on most or all of them. (GetAllPending... would not!) So then, you simply refactor by removing those methods and creating a generic version on the Repository<T>. This is just plain old vanilla extracting of a base class that you see all over the place, there's nothing magic about it just because it's relating to repositories.

Guessing

If you really don't like the idea of starting with an empty generic repository, there are some methods which using educated guesswork, you can be almost sure you'll need. So a starting point for your generic repository might be the following cut-down version of the one you posted:

public interface IGenericRepository<T> : where T : class
{
    IEnumerable<T> GetAll();
    T GetById(int id);
    void Add(T entity);
    void Save();
}

But to emphasise: don't just copy this without understanding why! Generic repositories are horribly badly explained in dozens of articles and blog posts across the internet, and it's really worth clearing up in your mind what they're actually for before using them.


Top 5 most common words in Python I would like to hear feedback from an interviewer's perspective on how to improve the following code:

from collections import Counter

def word_map(string):
    word_dict = {}
    stop_word_file=open("stopwords.txt", "r")
    stop_words =stop_word_file.read().split()
    for word in string.split():
        word = filter(str.isalnum, word).lower()
        word = word.strip()
        if word != '' and word not in stop_words:
            if word in word_dict.keys():
                word_dict[word] +=1
            else:
                word_dict[word] = 1

    return word_dict

file = open("story.txt", "r")

five_top_words = Counter(word_map(file.read())).most_common(5)
for letter, count in five_top_words:
    print '%s : %10d' %(letter, count)

I second @Dannnno arguments except for the fact that you don't need to split the work into tinier functions.

In fact I'd say quite the opposite as using more function let you turn them into generators for a more efficient memory usage.

You should also make yourself familiar with the str.format syntax which is the new way to go over the % formatting syntax.

My proposal is:

from collections import Counter
from itertools import chain

def word_map(filename, stop_words_file):
    with open(stop_words_file) as f:
        stop_words = set(f.read().split())

    with open(filename) as f:
        for line in f:
            words = (filter(str.isalnum, w).lower() for w in line.split())
            yield (w for w in words if w and w not in stop_words)

def top_words(filename, stopfile, count):
    words = chain.from_iterable(word_map(filename, stopfile))
    return Counter(words).most_common(count)

if __name__ == '__main__':
    for word, count in top_words('story.txt', 'stopwords.txt', 5):
        print '{} : {:>10}'.format(word, count)


Generic Pickle-Aware Super-Class for Mixing Classes with and without __slots__ in Python I want a generic super-class that I can easily subclass - and if desired add slots to my class - without having to worry about any pickle issues (with the lowest protocol). Is this in general a good idea? And if so, is this class a good implementation?

class HasSlots(object):
    """Top-class that allows mixing of classes with and without slots.

    Takes care that instances can still be pickled with the lowest
    protocol. Moreover, provides a generic `__dir__` method that
    lists all slots.

    """

    # We want to allow weak references to the objects
    __slots__ = ['__weakref__']

    def _get_all_slots(self):
        """Returns all slots as set"""
        all_slots = (getattr(cls, '__slots__', [])
                         for cls in self.__class__.__mro__)
        return set(slot for slots in all_slots for slot in slots)

    def __getstate__(self):
        if hasattr(self, '__dict__'):
            # We don't require that all sub-classes also define slots,
            # so they may provide a dictionary
            statedict = self.__dict__.copy()
        else:
            statedict = {}
        # Get all slots of potential parent classes
        for slot in self._get_all_slots():
            try:
                value = getattr(self, slot)
                statedict[slot] = value
            except AttributeError:
                pass
        # Pop slots that cannot or should not be pickled
        statedict.pop('__dict__', None)
        statedict.pop('__weakref__', None)
        return statedict

    def __setstate__(self, state):
        for key, value in state.items():
            setattr(self, key, value)

    def __dir__(self):
        result = dir(self.__class__)
        result.extend(self._get_all_slots())
        if hasattr(self, '__dict__'):
            result.extend(self.__dict__.keys())
        return result

Thus, I can simply sub-class this without worrying about slots and pickle:

class MyClass(HasSlots):
    def __init__(self, x,y):
        self._x = x
        self._y = y

but later maybe I want to optimize it and simply add some slots:

class MySlotClass(HasSlots):
    __slots__ = ['_x', '_y']
    def __init__(self, x,y):
        self._x = x
        self._y = y

And I still want to guarantee that my new object can be pickled:

import pickle
pickle.loads(pickle.dumps(MyClass(1,2))
pickle.loads(pickle.dumps(MySlotClass(1,2))

Are there any serious issues with my idea and implementation? Is this: all_slots = (getattr(cls, '__slots__', []) for cls in self.__class__.__mro__) a good approach to take care that all slots are properly pickled?

Is the approach safe for sub-classes that reference each other in a circular fashion?

A few of trivial points:

getattr(cls, '__slots__', []) would be better as getattr(..., ()) - tuples are immutable so are normally cached by the implementation.
set(slot for slots in all_slots for slot in slots) would be better as {slot for slots in all_slots for slot in slots}
It might be better to do

try:
    statedict = vars(self).copy()
except TypeError:
    statedict = {}


Finding the array indices of three specific headers I'm considering how to refactor code to make it clean & clear.

There are some principles to make code clean; in this case I think it's Single Responsibility and One level of abstraction.

The original code is like this:

for (int i = 0; i < header.length; i++) {
  if (header[i].equals(Constant.ID)) {
    indexOfID = i;
  } else if (header[i].equals(Constant.NAME)) {
    indexOfName = i;
  } else if (header[i].equals(Constant.CATEGORY)) {
    indexOfCategory = i;
  }
}

Then I separated the loop into smaller functions.

private int getIndexOfColumnType(String[] header, String type) {
  int index = -1;
  int i = 0;
  int size = header.length;

  while (index < size && index < 0) {
    if(header[i].equals(type)) {
      index = i;
    }
    i++;
  }
  return index;
}

And call it

int indexOfID = getIndexOfColumnType(header, Constant.ID);
int indexOfName = getIndexOfColumnType(header, Constant.NAME);
int indexOfCategory = getIndexOfColumnType(header, Constant.CATEGORY);

In this way, I need to use 3 loops; so I wonder which is better for performance
because header array can be large.

Please tell me your idea.

Both one-for-loop and getIndexOfColumnTypesolutions represent a naive approach: its main drawback is that for each index you need a dedicated variable like indexOfID or indexOfName.

Since you confirm that the header array can be quite large, this also means that you'll need as many variables for indices as there are items in the array.

Of course, there might be more flexible solutions and I see that the first step is already in the original code, supposing that Constant is an enumeration like this:

public enum Constant {
  ID,
  NAME,
  CATEGORY;
}

The index of each column can be mapped with a dedicated EnumMap:

Map<Constant, Integer> indices = new EnumMap<>(Constant.class);

Now this map can be filled with the indices, using a single loop and they will be accessible by the references to the respective Constants:

private void calculateColumnIndices() {
  for (int i = 0; i < header.length; i++) {
    try {
      Constant column = Constant.valueOf(header[i]);
      indices.put(column, i);
    } catch (IllegalArgumentException ex) {
      // handle the exception depending on what you expect to do if the header contains an invalid column name
      // TODO
    }
  }
}

To access the index:

int index = indices.get(Constant.CATEGORY);

P.S. I'd also suggest to rename Constant to something more meaningful, e.g. ColumnName.


Add items to a number of lists I am working on a rewrite project, where I'm supposed to change the
enhanced for-loop code used in java6 to the newer forEach loop.
Similarly, we have to use Streams API as heavily as possible.
Below is the complete scenario followed by code sample

Iterate over each element of List<String> list.
If Map<String, A> has each as key, take the corresponding A value from the Map.
A has an instance variable, List<B> which is a list of class B objects.
If List<B> is null initialize new List<B> and put some value inside it. Else just add a new value to Existing List<B>.

The complete code for the problem is below:
    class A{
        List<B> values;

    }

    class B{

    }

    //Assume some values
    List<String> list = ["val1","val2"];
    //Assume some values
    Map<String, A> map = { "val1" : [A object] , "val2" : [A object] };
    B b = new B();

    //Iterate over every value in list
    for(String each: list){
        //if map has a key with the value inside the list
        A a = map.get(each);

        //A has a list called "values" of B class objects. If that list is not null or empty add a new B object (b )in our case to the a List else initialze a new List and add the value there.
        if(a.values != null && !a.values.isEmpty()){
            a.values.add(b);
        }else{
            List<B> bList = new ArrayList<>();
            bList.add(b);
            a.values = bList;
        }
    }

Non sure what is the purpose to write your code in a java 8 way, please explain this part.

I think you mean using streams.

Anyway, here is an example.

Comparing with your code I introduced a filter that guarantee the key in list exists as a key in the map.

The other part are equivalent.

UPDATE: I added the _map_part that I had forgot previously.
The map can transform your list of string in a new list of different types.

In this case you use the list of string as a filter to get elements from the map. You don't want to operate on the String keys, instead you want to access the map values that are objects of type A.

list.stream()
    .filter(key -> (map.containsKey(key)))
    .map(key -> (map.get(key)))
    .forEach(a -> {
         if (a.values == null)
             a.values = new ArrayList<>();

         a.values.add(b);
     });

I think the main difference for this approach, compared with the old fashion is that you describe more your intentions, and the purpose of the code emerges much clearly.

But in trivial examples like yours the differences are not so clear and the functional approach seems much complicated.

Not all the case can be converted properly in this way, there are still cases where you whould prefer the loop over the streams.


Python class to implement a list with a size limit I am currently working through a python course about objected oriented programming. I was given this first assignment to work on:

  Create a simple class, MaxSizeList, that acts a little bit like a
  list, with a pre-configured limit on its size.

  Here some test calling code that imports the completed MaxSizeList and
  then uses it to create two new MaxSizeList objects.

from assignments import MaxSizeList  # assumes "class MaxSizeList"
                                     # is in a script called "assignments.py"

  Below are the test cases:

a = MaxSizeList(3)
b = MaxSizeList(1)

a.push("hey")
a.push("hi")
a.push("let's")
a.push("go")

b.push("hey")
b.push("hi")
b.push("let's")
b.push("go")

print(a.get_list())
print(b.get_list())

Output

 # ['hi', "let's", 'go']
 # ['go']

I came up with this code which outputs the correct result:

class MaxSizeList(object):

    ls = []

    def __init__(self,mx):
        self.val = mx

    def push(self,st):
        self.ls.append(st)

    def get_list(self):
        while len(self.ls) != self.val:
            self.ls.pop(0)
        return self.ls

Do you have any input on how this can be done more efficiently? Did I do it correctly or is there anything to note from the code I came up with?

Below,  I have slightly recast your class.  In those changes are incorporated two suggestions.

1) If limiting the size of the structure, do it at the input:

I removed the length check from get_list() and moved it to push()

2) Class attributes are not instance attributes:

You code had a bug whereby the ls attribute was declared on the class and not  on the instance. In the case of:

class MaxSizeList(object):

    ls = []

ls will be shared will all instances of MaxSizeList.  What you need is actually:

class MaxSizeList(object):

    def __init__(self, max_length):
        self.ls = []

In this case, every instance will get its own ls.

I am going to guess that this is why you needed to do the limiting on the output and not on the input.  That combined with using the same test data for the a and the b instance allowed your test cases to pass even with a substantial bug.

Code:

class MaxSizeList(object):

    def __init__(self, max_length):
        self.max_length = max_length
        self.ls = []

    def push(self, st):
        if len(self.ls) == self.max_length:
            self.ls.pop(0)
        self.ls.append(st)

    def get_list(self):
        return self.ls


File archiver, archive files into a zip file I've created a simple little file archiver that will run through a given directory and extract your files into an archive folder. It's pretty cool if I do so say myself, I would like some critique on what I've done, and some ideas on how I could better this program, better syntax, easier ways to do this, etc..

using System;
using System.IO;
using System.IO.Compression;

namespace ArchiveCreator
{
    class Program
    {
        //When program is run successfully
        //this will be the output
        public string Success(string input)
        {
            Console.ForegroundColor = ConsoleColor.White;
            Console.WriteLine(input);
            return input;
        }

        //When program encounters an error
        //this will be the output
        public string Warn(string input)
        {
            Console.ForegroundColor = ConsoleColor.Red;
            Console.WriteLine(input);
            return input;
        }

        //When program has information to show
        //this will be the output
        public string Say(string input)
        {
            Console.ForegroundColor = ConsoleColor.DarkCyan;
            Console.WriteLine(input);
            return input;
        }

        //Main method
        static void Main(string[] args)
        {
            //These variables are used to create a
            //random string that will be used as the
            //zip files name
            var chars = "abcdefghijklmnopqrstuvwyxzABCDEFGHIJKLMNOPQRSTUVWXYZ";
            var stringChars = new char[8];
            var random = new Random();

            //Info is used as provide the type of
            //information that will be displayed
            //by the program
            Program info = new Program();

            //Create the zip file name
            for (int i = 0; i < stringChars.Length; i++)
            {
                stringChars[i] = chars[random.Next(chars.Length)];
            }
            string finalString = new String(stringChars);

            info.Say("Starting file extraction..");

            string userName = Environment.UserName;
            string startDir = $"c:/users/{userName}/test_folder";
            string zipDir = $"c:/users/{userName}/archive/{finalString}.zip";
            string dirName = $"c:/users/{userName}/archive";

            //Check if the directory exists
            if (Directory.Exists(dirName))
            {
                info.Say("Directory already exists, resuming");
            }
            else
            {
                //Create it if it doesn't
                Directory.CreateDirectory(dirName);
            }

            try
            {
                ZipFile.CreateFromDirectory(startDir, zipDir);
            }
            catch (Exception e)
            {
                info.Warn($"Error: {e}");
            }
            info.Success($"Extracted files successfully to: {zipDir}");
            info.Say("Press enter to exit..");
            Console.ReadLine();
        }
    }
}

Code organization

The Main method should be very short, and have almost no logic at all, simply call other methods and create very few objects. Something like this:

static void Main(string[] args)
{
    string filename = archiveCreator.GenerateRandomFilename();

    string userName = Environment.UserName;
    string startDir = $"c:/users/{userName}/test_folder";
    string zipFile = $"c:/users/{userName}/archive/{finalString}.zip";
    string dirName = $"c:/users/{userName}/archive";

    Say("Starting file extraction..");

    var archiveCreator = new ArchiveCreator();
    if (archiveCreator.CreateDirIfNotExists(dirName)) {
        archiveCreator.CreateArchive(startDir, zipFile);
    }

    Say("Press enter to exit..");
    Console.ReadLine();
}

Printing messages

This and the similar methods simply print messages:

public string Success(string input)
{
    Console.ForegroundColor = ConsoleColor.White;
    Console.WriteLine(input);
    return input;
}

As such, these have nothing to do with the logic of creating archives,
so these methods would be better in another class.
Or, you could keep them within this class, but make them static.

Also, I don't see a point in return input,
it would be better to make these methods void.

Error handling

Consider this piece:

try
{
    ZipFile.CreateFromDirectory(startDir, zipDir);
}
catch (Exception e)
{
    info.Warn($"Error: {e}");
}
info.Success($"Extracted files successfully to: {zipDir}");

If an exception occurs in the try block,
the program prints a warning,
but it also prints "Extracted files successfully",
which is just not true.
Probably you want to move that line inside the try block.

Also, the program doesn't check that the directory could be created. It should.

Naming

Symbols in a program should have good names,
to make it easier for readers to understand the program and how the different elements work together.
For example, stringChars, Program, info don't describe well what they are:

var stringChars = new char[8];

Program info = new Program();

I recommend renaming them, for example:

stringChars -> filename or randomFilename
Program -> ArchiveCreator
info -> archiveCreator


Avoid indexing the keys in a Javascript array I need a data structure to store feeds in columns into tabs.

So the pseudo structure would be:

First Tab : name (string)
    First column
        First feed : (url (str), type (str), limit (int))
        Second feed : (url (str), type (str), limit (int))
    Second column
        First feed : (url (str), type (str), limit (int))
        Second feed : (url (str), type (str), limit (int))
        Third feed : (url (str), type (str), limit (int))
Second Tab : name (string)
    First column
        First feed : (url (str), type (str), limit (int))
    Second column
        First feed : (url (str), type (str), limit (int))

... And so on.

Now this works:

  var feeds = [
    {"groupName":"News",
     "0": [
       {"url":"http://xkcd.com/rss.xml","type":"mixed","limit": 8},
       {"url":"http://xkcd.com/rss.xml","type":"photo","limit": 4}
     ],
     "1": [
       {"url":"http://xkcd.com/rss.xml","type":"mixed","limit": 8},
       {"url":"http://xkcd.com/rss.xml","type":"mixed","limit": 8},
       {"url":"http://xkcd.com/rss.xml","type":"photo","limit": 4}
     ]
    },
    {"groupName":"Comics",
     "0": [
       {"url":"http://xkcd.com/rss.xml","type":"photo","limit": 8},
       {"url":"http://superphazed.tumblr.com/rss","type":"photo","limit": 8}
     ]
    }
  ];

But

I want to avoid maintaining a numerical index for the column keys ;
I'm not sure it's the right data structure type.

Apparently the form above is a JavaScript literal object, that is, not an array and not JSon. This is a problem, I want to use the right data structure for what I intend to do with it : At one point in time, I need to validate variables against this data structure, that is to say "OK, this variable is a proper list of feeds". I also need to easily add, delete and rename elements, and from what I can see, there are at least 3 approaches to "arrays" in JavaScript, each with its own set of idiosyncrasies, methods and tools, dos and don'ts ; The context is JavaScript (module pattern) in the browser, and JQuery is available.

So I guess my question is twofold:

What is the best data structure in this context to allow easy manipulation later ;
Is there a way to have unnamed columns containing non-indexed feeds?

You don't need to label object fields 0 or 1. On each Tab, you could have a field columns which is an array, of arrays containing the feeds in the column as so:

const Tabs = [
  {
    groupName: "News"
    columns: [
      [
        {"url":"http://xkcd.com/rss.xml","type":"mixed","limit": 8},
        {"url":"http://xkcd.com/rss.xml","type":"photo","limit": 4}
      ],
      [
        {"url":"http://xkcd.com/rss.xml","type":"mixed","limit": 8},
        {"url":"http://xkcd.com/rss.xml","type":"mixed","limit": 8},
        {"url":"http://xkcd.com/rss.xml","type":"photo","limit": 4}
      ]
    ]
  },
  {
    groupName: "Comics",
    columns: [
      [
        {"url":"http://xkcd.com/rss.xml","type":"photo","limit": 8},
        {"url":"http://superphazed.tumblr.com/rss","type":"photo","limit": 8}
      ]
    ]
  }
];


Ruby OOP convention/layout I am new to programming and I have just written my first program. It takes files from an SD card and copies then into a new named folder and has the option to name the files themselves or simply move them into the named folder.

As I am new I am unsure of the correct convention in terms of layout or structure of a program. Would a 'real' program look like this and how should it be structured?

Dir.chdir '/home/moot/pickaxe/pics' #replace this with destination directory

$pic_names = Dir['/media/moot/**/*.{CR2,cr2,JPG,jpg}'] #replace this with sd card directory

def folder
 puts "Enter the folder name"
 while $folder_name = gets.chomp
 if File.exist?("#{$folder_name}")
   puts "This folder already exists, choose a unique name"
 else
   require 'fileutils'
   FileUtils::mkdir_p "/home/moot/pickaxe/pics/#{$folder_name}"
   break
   end
  end
end

def files
  puts "Do you want to name the files? Answer y/n"
  while question = gets.chomp
    if question == "y"
      puts "What do you want to call them?"
      $file_name = gets.chomp
      print "Downloading #{$pic_names.length} Files: "
      pic_number = 1
      $pic_names.each do |name|
      print 'ˁ˚ᴥ˚ˀ  '
      new_name = if pic_number < 10
      "#{$file_name} 0#{pic_number} .CR2"
    else
      "#{$file_name} #{pic_number} .CR2"
    end
    require 'fileutils'
    include FileUtils
    cp(name, new_name)
    pic_number = pic_number + 1
    end
    break
    elsif question == "n"
    print "Downloading #{$pic_names.length} Files: "
        $pic_names.each do |name|
        print 'ˁ˚ᴥ˚ˀ  '
    new_dir = "/home/moot/pickaxe/pics/#{$folder_name}"
    require 'fileutils'
    include FileUtils
    cp(name, new_dir)
    end
    break
    else
      puts "Please answer y/n"
    end
  end
end

folder
Dir.chdir "/home/moot/pickaxe/pics/#{$folder_name}"
files
puts "Move Complete."

I started to write a detailed review, but in middle of it I realised that best advise I could give you looking at this code is to pick up some good Ruby book and work through it carefully, paying attention to how examples are formed. You seem to understand basics of how programming works, but your code lacks proper usage of many basic concepts, like arguments, returning, modules etc. Explaining them in detail here seems pointless, it's not a place for Ruby tutorials. You should focus on that basics before you move to object oriented design/conventions.

Still, I'll post what I already wrote as advices for you.

I don't really see any need to use OO structure in this code. It definately could still be written better though.

Indentation

Your way of indenting makes code very hard to read. Compare:

  $pic_names.each do |name|
  print 'ˁ˚ᴥ˚ˀ  '
  new_name = if pic_number < 10
  "#{$file_name} 0#{pic_number} .CR2"
else
  "#{$file_name} #{pic_number} .CR2"
end
require 'fileutils'
include FileUtils
cp(name, new_name)
pic_number = pic_number + 1
end

and:

$pic_names.each do |name|
  print 'ˁ˚ᴥ˚ˀ  '
  new_name =
    if pic_number < 10
      "#{$file_name} 0#{pic_number} .CR2"
    else
      "#{$file_name} #{pic_number} .CR2"
    end
  require 'fileutils'
  include FileUtils
  cp(name, new_name)
  pic_number = pic_number + 1
end

Ruby style guide contains some usefull examples on how you should indent your code to make it nice to read. Generally every block means 1 level of indentation (2 spaces).

Require

require 'fileutils'
include FileUtils

You only need to require 'fileutils' once, at top of your file. include also doesn't need to be reapated, instead generally it should be at top of a module.

Avoid globals

Instead of putting result of your methods in global variables, return from them. I.e:

def folder
  puts "Enter the folder name"
  while folder_name = gets.chomp
    if File.exist?("#{$folder_name}")
      puts "This folder already exists, choose a unique name"
    else
      require 'fileutils'
      FileUtils::mkdir_p "/home/moot/pickaxe/pics/#{$folder_name}"
      return folder_name
    end
  end
end

And than:

folder_name = folder
Dir.chdir "/home/moot/pickaxe/pics/#{folder_name}"

This is much more flexible.


Modeling a soccer team with substitutions I'm new to Python and want to make sure I'm not developing bad habits. If you could please review this code below and give me any tips, practice ideas or critiques you might have. Any ideas on which aspects of the language I should focus on early would also be appreciated.

#!usr/env/python
#Modeling a soccer team with substitutions

#Players stored in dictionary {Name:Numbers}
players = {}
# Player positions stored in a list
forward = []
midfield = []
defense = []
goalie = []

class Team(object):
    def __init__(self, name):
        self.name = name

class Players(object):
    def __init__(self):
        pass
#creates a player list with names, numbers, and preferred positions
    def addPlayer(self, name, number, position):
        self.name = name
        self.number = number
        self.position = position
        players[name] = number

        if position == 'forward':
            forward.append(name)
            return forward
        elif position == 'midfield':
            midfield.append(name)
            return midfield
        elif position == 'defense':
            defense.append(name)
            return defense
        elif position == 'goalie':
            goalie.append(name)
            return goalie
        else:
            print 'Only forward, midfield, defense, goalie accepted'

        print "Added %s to the team!" % name

#Should list the players currently on the field. Only 3 player sample data
players_on_field = []
class OnField(object):
        def __init__(self):
            pass
        def sub_in(self, sub_name):
            self.sub_name = sub_name
            if len(players_on_field) == 3:
                print "Field is full, sub_out first!"
                #testing some options
                #x = raw_input('Which player would you like to sub out?')
                #OnField.sub_out(x)
            else:
                players_on_field.append(sub_name)
                print "%s entering the game" % sub_name
            return

        def sub_out(self, sub_name):
            for i in range(3):
                if players_on_field[i] == sub_name:
                    del players_on_field[i]
                    print "%s has been removed from the game" % sub_name
                    return
                #testing some options
                #x = raw_input('Who would you like to sub in?')
                #OnField.sub_in(x)
            else:
                print "That player is not on the field"
                return

        def list_players_on_field(self):
            print players_on_field

#Testing
Barcelona = Team('Barcelona')
this = Players()
game = OnField()

this.addPlayer('Messi', 10, 'forward')
this.addPlayer('Neymar', 11, 'forward')
this.addPlayer('Song', 9, 'midfield')
this.addPlayer('Valdes', 1, 'goalie')
this.addPlayer('Pique', 25, 'defense')
this.addPlayer('Pedro', 8, 'forward')

print players

print 'Forward Players'
print forward
print 'Midfield Players'
print midfield
print 'Defense Players'
print defense
print 'Goalies'
print goalie

#Subsitution testing
print "Players currently on the field"
print players_on_field
game.sub_in('Messi')
game.sub_in('Pedro')
game.sub_in('Song')
game.list_players_on_field()
game.sub_out('Pedro')
print players_on_field
game.sub_in('Neymar')
print players_on_field

You naming convention does not follow PEP 8. Try to follow it as much as you can as it helps having same consistent guidelines all over Python code.

I do not quite understand how the on field thingy is supposed to work so I'll skip on that part.

Try to avoid global variables. Try to think how things would be affected if you were trying to handle multiple teams. Most probably things would go wrong.

I guess your different lists should be part of the team class. Also, there is not much point for having a Player class as a player on its own is nothing but a name. As far as I understand soccer, the number is for a given player in a given team. Thus, the team should be some kind of containers of pairs name/number.

Also, it seems like your are doing the same things for different teams of players that could be handled the same way. Instead of write code, it's easier to use smart data structures and make them work for you. In your case, it seems like a dictionnary from position to list of (player name + player number) is good idea. Default composition is to have empty lists and to fill them later on.

Here's what my code looks like :

class Team(object):
    def __init__(self, name):
        self.name = name
        self.composition = {
            'forward' : [],
            'midfield': [],
            'defense': [],
            'goalie': []
        }

    def add_player(self, name, number, position):
        if position not in self.composition:
            print 'Only %s accepted' % ', '.join(self.composition.keys())
        else:
            self.composition[position].append((name,number))
            print 'Added %s to the team!' % name

bcn = Team('Barcelona')
bcn.add_player('Messi', 10, 'forward')
bcn.add_player('Neymar', 11, 'forward')
bcn.add_player('Song', 9, 'midfield')
bcn.add_player('Valdes', 1, 'goalie')
bcn.add_player('Pique', 25, 'defense')
bcn.add_player('Pedro', 8, 'forward')
bcn.add_player('Josay', 10, 'invalid')

# It could be a good idea to create a method to do this :-)
for pos, players in bcn.composition.iteritems():
    print pos
    for p in players:
        print p


Calculate elapsed time from input times without using conditional statements I have just started programming and we are being taught C++. After our 2nd lecture we were given an assignment to make a program that would take two values of time from the user (0 <= hr <= 12 and 0 <= min <= 60)  and return the value of elapsed time.

Since conditionals statements are not introduced yet, I am trying to make the program without one.

#include<iostream.h>
#include<conio.h>
#include<math.h>
void main(void)
{
    clrscr();
    int ih,im,fh,fm,e,a,b;
    cout<<"Enter initial hour:";
    cin>>ih;
    cout<<"Enter initial min:";
    cin>>im;
    cout<<"Enter final hours:";
    cin>>fh;
    cout<<"Enter final min:";
    cin>>fm;
    a=ih*60+im;
    b=fh*60+fm;
    e=abs(((780+a)/(781+b))*720-abs(a-b));
    cout<<"the elasped time is "<<e/60<<":"<<e%60;
    getch();
}

I see several problems with my code. For one my code doesn't look nice and I would like to include hours and time in hr:min format. Furthermore, I think there are exceptions for which my code won't work. How can I sort these problems?

You may want to read Why is using namespace std bad practice?

#include<math.h>
void main(void)

Use whitespace to separate logical blocks of code:

#include<math.h>

int main(void)

Now you have an includes block and a code block.  This makes it easier to see where the function starts.

I also changed void main to int main.  While many compilers will accept void main, it is technically wrong.  The C++ standards specify that main returns an int.  Note that compilers will automatically return 0; for you.

int ih,im,fh,fm,e,a,b;

Try to avoid undescriptive names for variables.  Abbreviations make it easier to write code, but they slow down reading code.  You'll find that you spend more of your time reading code (including your own) than you do writing it.

int initial_hour, initial_minute, final_hour, final_minute;

I prefer to separate words in variable names with underscores.  Another common notation is camelCase, which capitalizes words after the first.

I don't see the a and b intermediate variables as necessary.  I moved my version of e lower.  More on that later.

std::cout << "Enter initial hour and minute separated by a space:";
std::cin >> initial_hour >> initial_minute;

If you're willing to use a space rather than a colon as the delimiter, you can read both on the same line in a relatively straightforward fashion.  There are methods that can handle a colon but they tend to require conditionals or more advanced data structures.  E.g. strptime returns a struct as a result.

I also added additional whitespace around all the tokens.  The compiler won't care, but it makes it easier for human readers to recognize the separate parts of the code.

a=ih*60+im;
b=fh*60+fm;

I'd write this

const int MINUTES_IN_AN_HOUR = 60;
initial_minute = initial_hour * MINUTES_IN_AN_HOUR + initial_minute;
final_minute = final_hour * MINUTES_IN_AN_HOUR + final_minute;

or actually

const int MINUTES_IN_AN_HOUR = 60;
initial_minute += initial_hour * MINUTES_IN_AN_HOUR;
final_minute += final_hour * MINUTES_IN_AN_HOUR;

But maybe you haven't covered the shorthand assignment operators yet.

I also declared a constant (which you may not have met yet) to hold the "magic value" 60.  See how it makes it more obvious what 60 represents?  If you don't want to use a constant yet, just leave off the const keyword.  It will work fine.  It's just that the compiler won't keep someone from modifying the value after you set it.

The constant would also make it easier to change things if we switched to hundred minute hours.  That seems unlikely in this case, but there are many other situations where that is valuable.  Note that we'd only have to change 60 to 100 in one place with the constant.  Without it, we'd have to switch in two places.  It's even more valuable when there are many uses scattered throughout the code.

Note that this way, you don't need the temporary variables a and b.

Does this work?

e=abs(((780+a)/(781+b))*720-abs(a-b));

It's not clear why you are doing the ((780+a)/(781+b))*720 part.  What's 780?  781?  720?  Why pick those numbers?  What are you trying to accomplish?  This is where comments come in helpful, as they can explain what you are doing to the reader.  Will you remember why you picked 780 and 781 in six months?

Did you perhaps mean to say something like

// add twelve hours to the elapsed time
// then take the time modulus twelve hours to mask out the extra twelve hours
// so 3 - 9 becomes 15 - 9 = 6 which stays 6
// while 9 - 3 becomes 21 - 3 = 18 which becomes 18 % 12 = 6
const int MINUTES_IN_TWELVE_HOURS = 720;
int minutes_elapsed = (MINUTES_IN_TWELVE_HOURS + final_minute - initial_minute) % MINUTES_IN_TWELVE_HOURS;

That would allow you to subtract 9 AM from 3 PM without having to specify AM/PM.  Note that it won't support an elapsed time of twelve hours or more.  It does this with only your existing operators.

Also notice that I declared minutes_elapsed (my replacement for your e variable) at the location it was used.  Now I don't have to go back to find out what type it is.  I can see it right there.

See how I used a comment to explain why I was doing this weird thing?  Now I don't have to figure it out again every time I read the code.

More Advanced

You probably haven't gotten here yet, since many classes don't bother to maintain good form, but most of your code should really be in a separate function.  Code in main is not reusable without copy and paste, but the concept of reading two times and subtracting them is.  So it would be better to put the code in another function or possibly two (one for input and one for calculations).


A simple Java login system I created a simple log-in system in Java for a project. Security isn't a concern here, hence the plaintext password, but I'm interested in how I can improve the quality of my code. Everything seems to work but it just feels like messy code.

public boolean logIn(String name, String password)
{
    if (getLoggedInUser() != null)
    {
        throw new IllegalArgumentException("You are already logged in");
    }

    boolean login = false;

    for (User user: getUsers())
    {
        if (user.toString().equals(name) && user.getPassword().equals(password))
        {
            loggedInUser = user;
            login = true;
        }
    }
    if (!login) //what if the username + password combo is not correct
    {
        for (User user: getUsers())
        {
            if (user.toString().equals(name))
            {
                throw new IllegalArgumentException("Your password is incorrect");
            }
        }
        throw new IllegalArgumentException("Your username is incorrect");
    }
    return login;
}

The Algorithm

The method logIn can be chunked into small steps:

  public boolean logIn(String name, String password) {

   if( /* user is logged in already? */)

   for (/* every user */)
     if ( /* has name and password */)

   if (/* no match found */)
     for (/* every user */)
       if ( /* can find user with the given name? */ )
}

The logic is more complex than it has to be. The algorithm searches twice if a user with the name exists. A simpler algorithm could look like:

public boolean logIn(String name, String password) {
    User user = // find user by its name

    if (/* no user found */)

    if (user.isLoggedIn)

    return user.hasPassword(password)
}

Data Structure

The method getUsers() looks like it returns a List. Since it is a list and you do not know at which index a concrete user is saved you have to search for the user.

  for (User user: getUsers())
{
  if (user.toString().equals(name))

To check that a user with name does not exists you have to loop through the hole list and this could take some time if you have many users! This is also known as \$O(n)\$ which is a indicator for the time complexity a algorithm can have.

It would be much more performant if we could get a user directly without to search it which would be a time complexity of \$O(1)\$.

We can archive this goal by using a Map instead of a List:

Map<String, User> nameByUser = new HashMap<>();

nameByUser.put("TomZ", new User("TomZ", "aPassw0rt"));
// ..insert some more users

User user = nameByUser.get("TomZ");


Stanford CS106A Min / Max program I am learning Java through Stanford CS106A and I'm wondering how the code for this early assignment looks.  I've worked in a proprietary scripting language before and so I'm trying to get the right programming form down for Java.  It works but am I doing anything in the wrong way?  Am I declaring and initializing my variables properly?  Is using a counter and conditional for the first instance like this good form? Am I missing a shorter, more efficient way?

/*
 * File: FindRange.java
 */

import acm.program.*;

public class FindRange extends ConsoleProgram {

    /* Specifies the value of the sentinel */
    private static final int SENTINEL = 0;

    public void run() {
        println("This program finds the largest and smallest numbers entered.");
        println("Enter values, one per line, using " + SENTINEL + " to signal the end of the list.");
        int min = 0;
        int max = 0;
        int count = 0;
        while (true) {
            int value = readInt(" ? ");
            if (value == SENTINEL) break;
            count++;
            if (count == 1){
                min = value;
                max = value;
            }
            min = Math.min(min, value);
            max = Math.max(max, value);
        }
        if (count>0){
            println("Smallest: " + min);
            println("Largest: " + max);
        } else {
            println("No values were entered.");
        }
    }
}

You could initialize min with Integer.MAX_VALUE and max with Integer.MIN_VALUE, this would make the inner if obsolete. You could also use this for your output condition and get rid of the count completely.

Without knowing the complete assignment, it's hard to optimize your code in any direction.

If you don't want to use your library and do it in 'plain' Java have a look at Scanner.


Finding maximum Collatz sequence between 1-1000000 I'm trying to find maximum the Collatz sequence between 1 and 1000000. I wrote the following code below. I guess it is correct but it is extremely slow. Can you give me a few hints to make it faster?

#include <iostream>
#include <vector>
#include <algorithm>
using namespace std;
bool myfn(int i, int j) { return i<j; }

int collatz(int x);

int main()
{
    vector <int> myvector;
    for(int i = 1; i < 1000000; i++)
    {
        myvector.push_back(collatz(i));
    }

    cout<<*max_element(myvector.begin(),myvector.end(),myfn);

    return 0;

}

int collatz(int x)
{
    int counter = 1;

    while(1)
    {
        if(x == 1)
            break;

        if(x % 2 == 0)
        {
            x = x / 2;
            counter++;
        }
        else
        {
            x = 3 * x + 1;
            counter++;
        }
    }

    return counter;
}

Your program is non-portable, since an int is only guaranteed to hold numbers up to 32767.  I would change your data type to unsigned long long; otherwise, collatz(159487) will overflow.

Your while condition is clumsy.

while(1)
{
    if(x == 1)
        break;
    …
}

should just be

while (x != 1)
{
    …
}

The key insight is that if you already know, for example, the value of collatz(37), then collatz(74) is just 1 + collatz(74 / 2), and you can reuse the previously computed result.  Therefore, your collatz() function should have access to the results vector — either

pass it in by reference, or
make the vector an instance variable, and the function a method of the class.

It turns out that recursion works quite well for this problem: every chain will terminate in a few hundred steps, so your function call chains will be at most a few hundred frames deep.  Using recursion allows you to cache the result for every number along the chain, which you can't do using iteration.

#include <iostream>
#include <limits>
#include <stdexcept>
#include <vector>

int collatz(unsigned long long n, std::vector<int> &results) {
    int c;
    if (n == 1) {
        return 1;
    } else if (n < results.size() && results[n]) {
        return results[n];
    } else if (n % 2 == 0) {
        c = 1 + collatz(n / 2, results);
    } else if (n >= (std::numeric_limits<unsigned long long>::max() - 1) / 3) {
        throw std::overflow_error("Overflow");
    } else {
        c = 1 + collatz(3 * n + 1, results);
    }
    if (n < results.size()) {
        results[n] = c;
    }
    return c;
}

int main() {
    const unsigned long long N = 1000000ULL;
    std::vector<int> results(N);
    int max = 0, max_i;
    for (unsigned long long i = 1; i < N; ++i) {
        results[i] = collatz(i, results);
        // std::cout << "collatz(" << i << ") = " << results[i] << std::endl;
        if (max < results[i]) {
            max = results[i];
            max_i = i;
        }
    }
    std::cout << "Max: " << max_i << " with " << max << " steps" << std::endl;
    return 0;
}

By packaging your code into a class, you can let main() not worry about some of the details:

template <typename T>
class Collatz {
  public:
    Collatz(T limit) : limit(limit), results(limit) {}

    int operator[](T n) {
        int c;
        if (n == 1) {
            return 1;
        } else if (n < results.size() && results[n]) {
            return results[n];
        } else if (n % 2 == 0) {
            c = 1 + (*this)[n / 2];
        } else if (n >= (std::numeric_limits<T>::max() - 1) / 3) {
            throw std::overflow_error("Overflow");
        } else {
            c = 1 + (*this)[3 * n + 1];
        }
        if (n < results.size()) {
            results[n] = c;
        }
        return c;
    }

    const T limit;

  private:
    std::vector<int> results;
};

int main() {
    Collatz<unsigned long long> collatz(1000000ULL);
    int max = 0, max_i;
    for (unsigned long long i = 1; i < collatz.limit; ++i) {
        // std::cout << "collatz(" << i << ") = " << collatz[i] << std::endl;
        if (max < collatz[i]) {
            max = collatz[i];
            max_i = i;
        }
    }
    std::cout << "Max: " << max_i << " with " << max << " steps" << std::endl;
    return 0;
}


Creating a backup of a website directory inside the Web Application Root directory in *nix In CentOS I have a Web Application Root on which I have a website directory.
I would like to make an immediate backup of that website's directory by using the zip utility. The code works and tested.

Although I already use set -x and zipping is verbose (I'm not sure if due to set -x or zip default behavior), the trace is super long and hard to follow - I cant vertically-scroll all of the way top of it through Putty Window in Windows10 Home.
I fear I don't have enough testing to ensure that the file was created, in what size (to assume the size is plausible) and such (maybe just adding ls -la ${war}/mediawiki_general_backups, is enough):

date="$(date +%F-%T)"
war="$HOME/public_html" # Web Application Root
domain="example.com"
zip -r "${war}/mediawiki_general_backups/${domain}-directory-backup-${date}.zip" "${war}/${domain}"

Well, to be honest I am not exactly sure what do you want to review :) This isn't a script. It is just standard zip command.

Maybe you should think about this: ${war}/mediawiki_general_backups/${domain}-directory-backup-${date}.zip. It is too long and ugly. Try to add more these things into your script and a readability will be lost.

  I fear I don't have enough testing to ensure that the file was created,

If you want to be sure, that the zip command finished sucessfully, just test it.

#!/usr/bin/env bash

# Any subsequent(*) commands which fail will cause the shell script to exit immediately.
# https://stackoverflow.com/a/2871034/10814755
# Now i should trust the output of zip command.
set -e

#domain=$1 <- It could be useful, isn't?
domain="example.com"

#I omit application root. I thing is safer to keep backups away.
backup_path="/mediawiki_backups/${domain}/"
backup_file="$(date +%F-%T).zip"

# You should trust the zip command, but sure is sure.
# So if backup file exist...
# ...and isn't empty...
if [[ -f ${backup_path}${backup_file} ]] && [[ ! -s ${backup_path}${backup_file} ]]; then
  #... then do something brave :)
else
  echo "Backup failed!"
  exit 1
fi

Regards :)


Simple dice roll using std::string as dice notation This will be a function that returns a random number from Dice Notation (more on Wikipedia).  I will later use this function to determine whether some skill check is passed or not.

Is there any need for optimization?
If so, how?
Any other thoughts about this code?

#include <string>

using namespace std;

int toInt(string text) {
    return atoi(text.c_str());
}

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

    srand(time(NULL));

    string diceString = "1d6+4";
    unsigned int dice1number = 0;

    string info = "dice ["+diceString+"] = %i\n";
    printf("==[START]================\n");

    printf(info.c_str(),dice1number);

    // parse dice
    unsigned int i=0;
    unsigned int part = 1;

    string dicePart1 = "";
    string dicePart2 = "";
    string dicePart3 = "";

    for(i=0;i<diceString.length();i++) {

        if (diceString[i]=='d') {
            part = 2;
            continue;
        }

        if (diceString[i]=='+') {
            part = 3;
            continue;
        }

        if (diceString[i]=='-') {
            dicePart3 += "-";
            part = 3;
            continue;
        }

        if (part==1 && diceString[i]>='0' && diceString[i]<='9') {
            dicePart1 += diceString[i];
        }

        if (part==2 && diceString[i]>='0' && diceString[i]<='9') {
            dicePart2 += diceString[i];
        }

        if (part==3 && diceString[i]>='0' && diceString[i]<='9') {
            dicePart3 += diceString[i];
        }

    }

    printf("dicePart1 = [%s]\n",dicePart1.c_str());
    printf("dicePart2 = [%s]\n",dicePart2.c_str());
    printf("dicePart3 = [%s]\n",dicePart3.c_str());

    int roll = 0;
    int add = toInt(dicePart3);
    roll = toInt(dicePart1) * (rand() % toInt(dicePart2)) + add;
    printf("roll = [%i]\n",roll);

    printf("==[END]==================\n");

    return 0;
}

Bug

Your code has an integer-division/multiplication problem/bug.

Consider the dice-roll specification 10d10+0.

Your code will parse this down to:

string dicePart1 = "10";
string dicePart2 = "10";
string dicePart3 = "0";

and this will be used as:

int add = toInt(dicePart3);
roll = toInt(dicePart1) * (rand() % toInt(dicePart2)) + add

At face value, this may be OK, but, in reality, the calculation boils down to:

roll = 10 * (rand() % 10) + 0;

This will only ever be able to produce 10 results, 0, 10, 20, 30, ...., 90, which really is not what you want.

You really should do this as a loop, or as a floating point operation.... the loop seems to be easier:

roll = toInt(dicePart3);
for (i = toInt(dicePart1); i > 0; i--) {
    roll += (rand() % toInt(dicePart2));
}

Second Bug

In addition to the bug I pointed out above, I have actually identified a second bug. Dice are always 1-based.... you cannot roll a '0'. So, the method would actually have to be:

roll = toInt(dicePart3);
for (i = toInt(dicePart1); i > 0; i--) {
    roll += 1 + (rand() % toInt(dicePart2));
}

Discussion on distribution...

You may be tempted to be more efficient and use the function:

int p1 = toInt(dicePart1);
int p2 = toInt(dicePart2);
int p3 = toInt(dicePart3);

roll = p1 + (rand() % (p1 * p2)) + p3;

The above will create a result that is distributed in the correct range.... but the actual distribution curve is flat.... the odds of a 10d10+0 being 10 is the same as it being 50.... but, in reality, there are many ways to throw a 50, but only 1 way to throw a 10. As a result, the roll-value of 10d10+0 is not evenly distributed in the range 10 ... 100. You need to use a method where each dice is rolled individually.

int p1 = toInt(dicePart1);
int p2 = toInt(dicePart2);
int p3 = toInt(dicePart3);

....

roll = p3;
for (i = p1; i > 0; i--) {
    roll += 1 + (rand() % p2);
}

The above will, for 10d10+0 produce values in the correct range 10 ... 100 and the value frequencies will be distributed normally, as expected.


Hearthstone fatigue calculator I made a simple Hearthstone fatigue calculator. For those who don't know, Hearthstone is a card game in which every turn you draw a card from your deck. Once you're out of cards, you start taking fatigue damage, getting 1 more damage each turn. The total damage dealt in X turns is known as the Triangular Number Sequence: 1 (1), 3 (1+2), 6 (1+2+3), 10 (1+2+3+4). For example, at turn 4 the fatigue damage has already dealt 10 dmg (1+2+3+4). This could be used for some decks that make you run out of cards, to calculate how much damage they need to kill the opponent or to just calculate how much damage X fatigue turns will do.

I just wanted to know how my code looks. I know it's a very simple program but I might be doing something wrong and I guess it's better to know as early as possible. Besides, code reviewing a long project is just too boring for everyone. I've been learning by myself and as nobody ever reads my code I might be doing terrible things without knowing it haha

def error():
    print("Whoops, something went wrong. Please try again.")

def calc_mode(t, s):
    d = 0
    for i in range(s, t+s):
        d = d + i
    return d

def lethal_mode(d_input, s):
    d_calc = 0
    i = 1
    t = 0
    while d_calc < d_input:
        d_calc = d_calc + i
        i += 1
        t += 1
    return t

while True:  # Checks if the mode input is valid
    try:
        mode = int(input("\nMODES: "
                         "\n* '1' to get the turns needed for X damage "
                         "\n* '2' to get the damage dealt in X turns "
                         "\nPlease type to select your mode: "))
    except ValueError:
        error()
        continue
    else:
        while True:
            if mode == 1:  # Checks if the input after the mode is valid
                while True:
                    try:
                        damageInput = int(input("\nPlease input the damage needed for lethal: "))
                        startInput = int(input("Starting at how much damage? (default is 1): "))
                    except ValueError:
                        error()
                        continue
                    else:

                        print("\n>> You will need %d fatigue turns for that sweet lethal"
                              % lethal_mode(damageInput, startInput))
                        continue

            if mode == 2:
                while True:
                    try:
                        turnsInput = int(input("\nPlease input how many fatigue turns: "))
                        startInput = int(input("Starting at how much damage? (default is 1): "))
                    except ValueError:
                        error()
                        continue
                    else:
                        print("\n>> It will deal %d damage" % calc_mode(turnsInput, startInput))
                        continue

            else:
                error()
                break

What do you think overall? Thanks!

The math

There is a useful formula for triangle numbers that vastly simplifies a lot of the code here:

$$\sum_{k=1}^n k = \frac{n(n+1)}{2}$$

So for instance, the first triangle number is \$1\$, the second \$1 + 2 = 3\$, the third \$1 + 2 + 3 = 6\$ and so fourth. Compare that to the Hearthstone fatigue counter.

The interface

The main function

In a lot of programming languages there is the concept of a main function. This is commonly emulated in Python with:

def main():
    ...

if __name__ == '__main__':
    main()

See this answer for more details. In your case, you should probably have your main() consist of the top level while True statement i.e.:

def main():
    while True:
        ...

if statements

It is rather unfortunate that Python doesn't have switch-case statements which provide a very nice way to match against mode. In this case I would recommend usage of tuple (or dictionary) to handle the modes. To illustrate:

def needed():
    while True:
        try:
            damageInput = int(input("\nPlease input the damage needed for lethal: "))
            startInput = int(input("Starting at how much damage? (default is 1): "))
            ...

def dealt():
    while True:
        try:
            turnsInput = int(input("\nPlease input how many fatigue turns: "))
            startInput = int(input("Starting at how much damage? (default is 1): "))
            ...

MODES = (needed, dealt)

def main():
    while True:
        ...
        MODES[mode]()

Default dictionary?

This takes a bit more work, but you probably want to invest some time into learning what defaultdict is. Default dictionaries give you the power of dictionaries in Python with the ability to avoid raising KeyErrors. You logic right now heavily relies on capturing bad input and handling the exceptions yourself. With a default dict, you:

don't need to convert anything to an integer, just use string keys. This prevents any sort of conversion error.
set the default value for the dictionary to error. In other words, MODES["SOME_GARBAGE_VALUE"]() will act like error().
Add another value to MODES which allows for an exit.


Simple Rust Webserver I started learning Rust today and for my first project created a simple webserver.

I don't have a clear vision about Rust programming and I tried to write it with my previous knowledge in Python and PHP.

use std::io::{Read, Write,BufReader, BufRead};
use std::net::{TcpListener, TcpStream};
use std::fs::File;
use std::path::Path;

fn main(){
    loop{
        let listener = TcpListener::bind("localhost:8000").unwrap();
        let stream  = listener.accept().unwrap().0;
        read_request(stream);
    }
}

fn read_request(stream: TcpStream){

    let mut lines = String::new();
    let mut reader = BufReader::new(stream);
    reader.read_line(&mut lines);
    let mut vec_line = lines.split_whitespace();
    let mut requested_page = vec_line.nth(1).unwrap().to_string();
    requested_page = requested_page.replace("/","");
    if requested_page == "" {
        requested_page = String::from("index.html");
    }
    let mut response = String::new();
    let path = Path::new(&requested_page);
    println!("{}",requested_page);
    let mut status = 200;
    if !path.exists(){
        response = String::from("Not Found!");
        status = 404;
    } else {
        let mut file = File::open(&requested_page).expect("Unable to open file");
        file.read_to_string(&mut response);
    }

    send_response(reader.into_inner(), &response.to_string(), status);

}

fn send_response(mut stream: TcpStream, res :&str, status :u32){
    let response = format!("{}{}{}{}","HTTP/1.1 ",status," OK\n\n",res);
    stream.write_all(response.as_bytes()).unwrap();
}

Learn to love rustfmt:

Spaces go between comma-separated items:

-use std::io::{Read, Write,BufReader, BufRead};
+use std::io::{Read, Write, BufReader, BufRead};

-let response = format!("{}{}{}{}","HTTP/1.1 ",status," OK\n\n",res);
+let response = format!("{}{}{}{}", "HTTP/1.1 ", status, " OK\n\n", res);

Spaces go before braces:

-fn main(){
+fn main() {

-loop{
+loop {

Colons attach on the left side of type definitions:

-fn send_response(mut stream: TcpStream, res :&str, status :u32){
+fn send_response(mut stream: TcpStream, res: &str, status: u32) {

Pay attention to compiler warnings. One of the entire purposes of using a compiled language is to have the compiler watch your back. If you ignore it, then what good is it?

warning: unused result which must be used, #[warn(unused_must_use)] on by default
  --> src/main.rs:19:5
   |
19 |     reader.read_line(&mut lines);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unused result which must be used, #[warn(unused_must_use)] on by default
  --> src/main.rs:35:9
   |
35 |         file.read_to_string(&mut response);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Introduce more functions to break up the distinct parts of read_request. From my inspection, there's a part that parses the request, a part that handles the request by reading a file, and a part that responds (which already exists). Note that doing this contrains the number of lines of code a variable is mutable and avoids the need to recover the inner TcpStream from the BufReader.
vec_line is poorly named; it's not a Vec. I am happy to see your use of nth; many people would collect into a Vec, which is inefficient.
There's no reason to convert the result of nth into a String, you can call replace on a &str equally as well.
Avoid using mut just to be able to reassign a variable (e.g. the result of replace). Instead, just bind the variable a second time. This reduces the amount and scope of mutability.
requested_page == "" should be requested_page.is_empty()
Why would you convert a String to a String using to_string? Just take a reference to the existing string.
There's no need to create a string to write to the stream; just write! directly to the stream.
The code always prints "OK", regardless of the status.
Checking if a file exists before reading it introduces a race where the file can be deleted between the check and the read. It's better to open the file and check any resulting error.
Instead of creating mutable variables (response, status) with defaults, consider only setting them in the appropriate branches. Again, this reduces the number of mutable variables and the scopes of mutability.
Once response and status are moved to a function, you can just return them directly.
read_request does more than just read, so it should be renamed.
The current code doesn't require a TcpStream except in the outermost loop. It only needs anything that implements Read and Write. Using generics will allow you to more easily test those functions in isolation.

use std::io::{self, Read, Write, BufReader, BufRead};
use std::net::TcpListener;
use std::fs::File;

fn main() {
    loop {
        let listener = TcpListener::bind("localhost:8000").unwrap();
        let stream = listener.accept().unwrap().0;
        handle_request(stream);
    }
}

fn handle_request<S>(mut stream: S)
    where S: Read + Write
{
    let requested_page = parse_request(&mut stream);
    let (response, status) = read_file(&requested_page);
    send_response(stream, &response, status);
}

fn parse_request<R>(stream: R) -> String
    where R: Read
{
    let mut line = String::new();

    let mut reader = BufReader::new(stream);
    reader.read_line(&mut line).unwrap();

    let mut words = line.split_whitespace();
    let requested_page = words.nth(1).unwrap();
    let requested_page = requested_page.replace("/", "");

    if requested_page.is_empty() {
        String::from("index.html")
    } else {
        requested_page
    }
}

fn read_file(requested_page: &str) -> (String, u32) {
    match File::open(&requested_page) {
        Ok(mut file) => {
            let mut s = String::new();
            file.read_to_string(&mut s).unwrap();
            (s, 200)
        }
        Err(ref e) if e.kind() == io::ErrorKind::NotFound => (String::from("Not Found!"), 404),
        Err(e) => panic!("Unable to open file: {}", e),
    }
}

fn send_response<W>(mut stream: W, res: &str, status: u32)
    where W: Write
{
    write!(&mut stream, "{}{}{}{}", "HTTP/1.1 ", status, " OK\n\n", res).unwrap();
}

There are way too many uses of unwrap. At the very least, use expect so you have some clue about what caused the program to terminate. Even better, use Result for error handling.

As a basic step, returning a Box<Error> is extremely simple and allows you to consolidate your error handling:

use std::io::{self, Read, Write, BufReader, BufRead};
use std::net::TcpListener;
use std::fs::File;

type Error = Box<std::error::Error>;
type Result<T> = std::result::Result<T, Error>;

fn main() {
    loop {
        if let Err(e) = handle_one("localhost:8000") {
            panic!("An error occurred, I'm exiting! {}", e);
        }
    }
}

fn handle_one(address: &str) -> Result<()> {
    let listener = TcpListener::bind(address)?;
    let stream = listener.accept()?.0;
    handle_request(stream)
}

fn handle_request<S>(mut stream: S) -> Result<()>
    where S: Read + Write
{
    let requested_page = parse_request(&mut stream)?;
    let (response, status) = read_file(&requested_page)?;
    send_response(stream, &response, status)
}

fn parse_request<R>(stream: R) -> Result<String>
    where R: Read,
{
    let mut line = String::new();

    let mut reader = BufReader::new(stream);
    reader.read_line(&mut line)?;

    let mut words = line.split_whitespace();
    let requested_page = words.nth(1).ok_or("request missing path")?;
    let requested_page = requested_page.replace("/", "");

    if requested_page.is_empty() {
        Ok(String::from("index.html"))
    } else {
        Ok(requested_page)
    }
}

fn read_file(requested_page: &str) -> Result<(String, u32)> {
    match File::open(&requested_page) {
        Ok(mut file) => {
            let mut s = String::new();
            file.read_to_string(&mut s)?;
            Ok((s, 200))
        },
        Err(ref e) if e.kind() == io::ErrorKind::NotFound => {
            Ok((String::from("Not Found!"), 404))
        }
        Err(e) => Err(e.into()),
    }
}

fn send_response<W>(mut stream: W, res: &str, status: u32) -> Result<()>
    where W: Write,
{
    write!(&mut stream, "{}{}{}{}", "HTTP/1.1 ", status, " OK\n\n", res)?;
    Ok(())
}


Gather all IP addresses of a host as a vector I have the following working code to get IPv4/v6 addresses from host names:
#include <array>
using std::array;

#include <iostream>
using std::cout;

#include <netdb.h>

#include <stdexcept>
using std::domain_error;

#include <sys/socket.h>

#include <string>
using std::string;

#include <vector>
using std::vector;

#include <boost/asio.hpp>
using boost::asio::ip::address;
using boost::asio::ip::address_v4;
using boost::asio::ip::address_v6;

vector<address> getAddresses(string const &hostname)
{
    struct addrinfo req = {.ai_family = AF_UNSPEC, .ai_socktype = SOCK_STREAM};
    struct addrinfo *pai;
    int error = getaddrinfo(hostname.c_str(), nullptr, &req, &pai);
    if (error)
        throw domain_error("Could not resolve host name.");

    vector<address> addresses;

    for(struct addrinfo *info = pai; info != nullptr; info = info->ai_next) {
        if (info->ai_family == AF_INET) {
            auto ipv4socket = reinterpret_cast<struct sockaddr_in*>(info->ai_addr);
            auto ipv4addr = address_v4(htonl(ipv4socket->sin_addr.s_addr));
            addresses.emplace_back(ipv4addr);
        } else {
            auto ipv6socket = reinterpret_cast<struct sockaddr_in6*>(info->ai_addr);
            auto ipv6base = reinterpret_cast<unsigned char*>(ipv6socket->sin6_addr.s6_addr);
            array<unsigned char, 16> bytearray = {
                ipv6socket->sin6_addr.s6_addr[0],
                ipv6socket->sin6_addr.s6_addr[1],
                ipv6socket->sin6_addr.s6_addr[2],
                ipv6socket->sin6_addr.s6_addr[3],
                ipv6socket->sin6_addr.s6_addr[4],
                ipv6socket->sin6_addr.s6_addr[5],
                ipv6socket->sin6_addr.s6_addr[6],
                ipv6socket->sin6_addr.s6_addr[7],
                ipv6socket->sin6_addr.s6_addr[8],
                ipv6socket->sin6_addr.s6_addr[9],
                ipv6socket->sin6_addr.s6_addr[10],
                ipv6socket->sin6_addr.s6_addr[11],
                ipv6socket->sin6_addr.s6_addr[12],
                ipv6socket->sin6_addr.s6_addr[13],
                ipv6socket->sin6_addr.s6_addr[14],
                ipv6socket->sin6_addr.s6_addr[15]
            };
            auto ipv6addr = address_v6(bytearray, ipv6socket->sin6_scope_id);
            addresses.emplace_back(ipv6addr);
        }
    }

    return addresses;
}

int main()
{
    auto addresses = getAddresses("heise.de");

    for (auto ipa : addresses)
        cout << "Address: " << ipa << "\n";

    return 0;
}

I find the conversion fom uint8_t[16] (ipv6socket->sin6_addr.s6_addr) to std::array<unsigned char, 16> a bit verbose.
I also considered doing it in a for-loop, but this would still seem cumbersome to me, somehow.
I tried static_cast and reinterpret_cast to no avail.
Is there a possibility to do this conversion in a simpler way, preferably a one-liner?

I agree with you that the copying of data from s6_addr seems cumbersome.  I would instead just cast without copying:
auto& bytearray = reinterpret_cast<std::array<unsigned char,16>&>(ipv6socket->sin6_addr.s6_addr);

(If we had needed to copy, we could change auto& to plain auto).
Apart from this, the variable ipv6base is never used, so should just be removed.


Dijkstra's algorithm in Python I was hoping that some more experienced programmers could help me make my implementation of Dijkstra's algorithm more efficient.

So far, I think that the most susceptible part is how I am looping through everything in X and everything in graph[v].

My graph is formatted as:

g = {0:{1:2}, 1:{0:2, 2:6}, 2:{1:6}}

This is my full code, where n is the number of vertices and m is the number of edges, formatted like this:

n m
v1 v2 weight
...

from sys import stdin
n, m = stdin.readline().split()
n, m = int(n), int(m)
graph = {i:{} for i in range(n)}
V = [i for i in range(n)]

# paths to themselves have zero length
for i in range(m):
    a, b, t = stdin.readline().split()
    a, b, t = int(a), int(b), int(t)
    graph[a][b] = t
    graph[b][a] = t

def Dijkstra(graph, start):
# places we've found shortest path for
X = [start]
# list of shortest path length to vertices
A = [0]*len(graph)

    while X != V:
        #Dijkstra's greedy criterion
        U = float('inf')
        W = float('inf')
        uw = float('inf')
        for v in X:
            for w in graph[v]:
                if A[v] + graph[v][w] < uw and w not in X:
                    uw = A[v] + graph[v][w]
                    U = v
                    W = w
        X.append(W)
        try:
            A[W] = uw
        except:
            return A

A = Dijkstra(graph, 0)
B = Dijkstra(graph, n-1)
C = [A[i] + B[i] for i in range(n)]
print(max(C))

(I'm assuming the code will be changed according to the comments. Otherwise it won't run with the given example graph)

Performance issues:

Comparing lists as in while X != V involves looping through the lists. Also, the condition is not very useful because the lists only become equal in the special case when the algorithm visits the vertices in numerical order. You could as well use while True because the exception you are catching will occur when there are no vertices left to explore.
The w not in X check also loops through X. Making X a set would help with that.
After visiting each vertex, the for loops go through all the edges from all visited vertices, computing the tentative distances. That's a lot of repeated work. The usual approach is to compute the tentative distances only from the vertex just visited to its neighbors and store them in a data structure that allows querying the minimum distance. In Python the heapq module is available to help with that.

Implementation using heapq:

from heapq import heappush, heappop

def Dijkstra(graph, start):
    A = [None] * len(graph)
    queue = [(0, start)]
    while queue:
        path_len, v = heappop(queue)
        if A[v] is None: # v is unvisited
            A[v] = path_len
            for w, edge_len in graph[v].items():
                if A[w] is None:
                    heappush(queue, (path_len + edge_len, w))

    # to give same result as original, assign zero distance to unreachable vertices
    return [0 if x is None else x for x in A]


find first bad version Working on below find first bad version problem, post my code in Python 2.7, any smarter ideas for better time complexity, any code bug or code style advice are highly appreciated. My major idea is divide and conquer like a binary search.

Problem,

You are a product manager and currently leading a team to develop a new product. Unfortunately, the latest version of your product fails the quality check. Since each version is developed based on the previous version, all the versions after a bad version are also bad.
Suppose you have n versions [1, 2, ..., n] and you want to find out the first bad one, which causes all the following ones to be bad.
You are given an API bool isBadVersion(version) which will return whether version is bad. Implement a function to find the first bad version. You should minimize the number of calls to the API.

Source code,

def isBadVersion(number):
    if number == 0:
        return False
    else:
        return True
# 0 means good, 1 means bad
def find_first_bad_index(numbers):
    start = 0
    end = len(numbers)-1
    if isBadVersion(numbers[start]) == True:
        return start
    if isBadVersion(numbers[end]) == False:
        raise Exception('all good')
    while start <= end:
        mid = start + (end-start)/2
        if isBadVersion(numbers[mid]) == True:
            if start == end:
                return end
            else:
                end = mid
        else:
            start = mid + 1

if __name__ == "__main__":
    print find_first_bad_index([0,0,0,1,1,1,1,1])
    print find_first_bad_index([0,0,0,0,0,0,0,1])

Obligatory PEP 8 reminder:

Whitespace is important.
Whitespace is important too.
Recommendations state that you should:

Design exception hierarchies based on the distinctions that code catching the exceptions is likely to need, rather than the locations where the exceptions are raised. Aim to answer the question "What went wrong?" programmatically, rather than only stating that "A problem occurred" (see PEP 3151 for an example of this lesson being learned for the builtin exception hierarchy);
Not compare boolean values to True or False using == .

Yes:   if greeting:
No:    if greeting == True:
Worse: if greeting is True:

Applying that to your code gives the more readable:

class NoBadVersion(Exception):
    pass

def isBadVersion(number):
    """Mocked external API. Obligatory camelCaseName for consistency."""
    return number > 0

def find_bad_version(numbers):
    """Return the index of the first bad version.
    Tries to perform as few external API calls as possible.
    """

    start = 0
    end = len(numbers) - 1

    if isBadVersion(numbers[start]):
        return start

    if not isBadVersion(numbers[end]):
        raise NoBadVersion('all good')

    while start <= end:
        mid = start + (end-start) / 2
        if isBadVersion(numbers[mid]):
            if start == end:
                return end
            end = mid
        else:
            start = mid + 1

if __name__ == "__main__":
    print find_bad_version([0, 0, 0, 1, 1, 1, 1, 1])
    print find_bad_version([0, 0, 0, 0, 0, 0, 0, 1])


Summarizing calories for a food diary I'm new to C and I've been working on some code for a project for school. This was the finished result and now that I'm done it, I came back and tried making it better. So I was wondering, how would I make this printAvgCalPerGroup simplified?

#include "diary.h"

void printTotalCal(Food * list)
{
     int total = 0;
     Food * temp = list;

     while (!isEmpty(temp))
     {
          total += (int)temp->calories;
          temp = temp->next; // change to the next list item
     }

     printf("%d\n", total);
     return;
}

void printAvgCalPerGroup(Food * list)
{
     double vegCount = 0.0;
     double meatCount = 0.0;
     double dairyCount = 0.0;
     double grainsCount = 0.0;
     double fatCount = 0.0;

     double vAmnt = 0.0;
     double mAmnt = 0.0;
     double dAmnt = 0.0;
     double gAmnt = 0.0;
     double fAmnt = 0.0;

     Food * temp = list;

     while (temp!= NULL)
     {
          if (temp->foodGroup[0] == 'v')
          {
               vAmnt+=1.0;
               vegCount+=temp->calories;
          }
          else if (temp->foodGroup[0] == 'm')
          {
               mAmnt+=1;
               meatCount+=temp->calories;
          }
          else if (temp->foodGroup[0] == 'd')
          {
               dAmnt+=1;
               dairyCount +=temp->calories;
          }
          else if (temp->foodGroup[0] == 'g')
          {
               gAmnt+=1;
               grainsCount+=temp->calories;
          }
          else if (temp->foodGroup[0] == 'f')
          {
               fAmnt+=1;
               fatCount+=temp->calories;
          }
          temp = temp->next;
     }
     printf("%.2f\n", calcAvg(vegCount,vAmnt));
     printf("%.2f\n", calcAvg(meatCount,mAmnt));
     printf("%.2f\n", calcAvg(dairyCount,dAmnt));
     printf("%.2f\n", calcAvg(grainsCount,gAmnt));
     printf("%.2f\n", calcAvg(fatCount,fAmnt));

}

double calcAvg(double x, double y)
{
     double ans = x/y;
     if (ans != ans) return 0.0;
     return ans;
}

void run(char * argv)
{
     Food * f = getDiary(argv);
     printTotalCal(f);
     printAvgCalPerGroup(f);
     printList(f);
     destroyList(f);
}

Reducing repetition
The big if statement in printAvgCalPerGroup is a bit repetitive (as you pointed out).  Imagine if you increased the number of food groups.  You would have to copy/paste 4 places for each food group you added.
First, I would suggest using arrays for the counts and amounts of each foodgroup.  Then you only have to convert the letter such as 'v' into an array index.
Second, if the food groups could possibly change, I would suggest using an enum to keep track of them.  This is definitely optional, and you could use hardcoded numbers instead (e.g. 'v' -> 0, 'm' -> 1, etc).
Third, a small change to make would be to track the amounts using ints instead of doubles, since the amounts always increase by 1 only.
Here is what your code would look like using arrays and enums:
typedef enum FoodGroup {
    VEG,
    MEAT,
    DAIRY,
    GRAINS,
    FAT,
    FOODGROUP_MAX
} FoodGroup;

void printAvgCalPerGroup(Food * list)
{
    double    count[FOODGROUP_MAX] = {0};
    int       amnt [FOODGROUP_MAX] = {0};
    FoodGroup group;

    Food * temp = list;

    while (temp!= NULL) {
        switch (temp->foodGroup[0]) {
            case 'v': group = VEG;           break;
            case 'm': group = MEAT;          break;
            case 'd': group = DAIRY;         break;
            case 'g': group = GRAINS;        break;
            case 'f': group = FAT;           break;
            default : group = FOODGROUP_MAX; break;
        }
        if (group < FOODGROUP_MAX) {
            amnt [group]++;
            count[group] += temp->calories;
        }
        temp = temp->next;
    }

    for(group=0; group<FOODGROUP_MAX; group++)
        printf("%.2f\n", calcAvg(count[group], amnt[group]));
}

If you don't need the enum, just replace VEG with 0, MEAT with 1, etc.


Reverse lookup ID by value Is there a faster way of getting the key of 1 or 2 by using the accs value for my getID function? Unfortunately I cannot use an inverse dict due to the fact I'm using a list for the value which causes a unhasable list error. Mainly I'm worried more when the db gets larger in size the getID function will take longer to loop through the db.

db = {"users":{"1":{"main":"charles","accs":["charles","ashketchum"],"rank":4},"2":{"main":"don","accs":["don","donny"],"rank":4}}}

class user_data:

    def get(name, key):
        key = key.lower()
        if key in db["users"][name]:
            return db["users"][name][key]
        else:
            return None

    def getID(name):
        name = name.lower()
        for key in db["users"].keys():
            if name in db["users"][key]["accs"]:
                return key
        return None

Note: the accs value will change as well.

Why not use a set? For a set in is O(1). You can extend the set by adding another acc with db['users']['1']['accs'].add('misty') (add instead of append). After this change your lookup is only O(n), where n is the number of users (at worst) and in reality will depend on where the user is in the internal representation of the dict.

Alternatively you could just use a tuple (which is hashable). This just makes it different to add another account to a user:

db['users']['1']['accs'] += ("misty",)

This way you could build the reverse dictionary, speeding the lookup up to O(1). However, you would have to keep the reversed and actual db in sync.

In addition, to iterate over the keys of a dict, it is sufficient to do:

for key in db['users']:

But here I  would actually do:

for key, user in db['users'].items():
    if name in user['accs']:
        return key

You don't need the explicit return None because None is already the default return value of a function in python.

Your function get(name, key) is confusing, because name is actually id (so "1", "2", ...).

Python has an official style guide, PEP8. One of the things it recommend is using lower_case for variable and function names.


dnaof() - inheritance made easy, my own make inheritance tool https://github.com/exebook/dnaof

I created this simple inheritance tool for JavaScript, could any one with deep knowledge in JavaScript prototyped inheritance review it?

Here is the library itself:

kindof = function(K) {
    function X() {}
    if (K != undefined) X.prototype = new K
    X.can = X.prototype
    return X
}
dnaof = function(x, f) {
    var p = x.__proto__
    x.__proto__ = p.__proto__
    var r = p.__proto__[f].apply(x)
    x.__proto__ = p
    return r
}

And here is the example usage to demonstrate what it can do:

require('./dnaof')

// create a kind of idiot without ancestor:

var idiot = kindof()

// tell what it can do:

idiot.can.say = function() { return this.name + ' can chat' }
idiot.can.rest = function() { console.log('bzzzz.z.z.z... (' + this.name + ')') }

// a new kind of smart inherits from a kind of idiot

var smart = kindof(idiot)

// he can also say something:

smart.can.say = function() {
    // he can say something new, and he can say the same thing as an idiot can:
    return dnaof(this, 'say') + ', ' + this.name + ' can talk'
}

// a kind of a genious can do the same things as an idiot and smart can, and even more:

var genious = kindof(smart)
genious.can.say = function() {
    return dnaof(this, 'say') + ', ' + this.name + ' can discuss'
}

// instantiate three persons:

var bob = new idiot
var alice = new smart
var candy = new genious

// assign properties, because to make life simpler we do not initialize anything during creation:

bob.name = 'bob'
alice.name = 'alice'
candy.name = 'candy'

// let them talk:

console.log(bob.say())
console.log(alice.say())
console.log(candy.say())

// let them take some rest:

alice.rest(), bob.rest(), candy.rest()

From a once over, and borrowing from the comments:

__proto__ is bad news, MDN mentions this in a big red box of doom.
There is no means to pass parameters to the constructor, that does not make it simple
X, x, k, r and p are terrible variable names
dnaof is an unfortunate name
lowerCamelCase is good for you, kindof -> kindOf,
You can get the parent class thru Object.getPrototypeOf(Object.getPrototypeOf(this)) in dnaof
I have mixed feelings about the can trick. It is shorter than typing prototype, but it's also a candidate for nameclashes.
You have no comments at all in your code, combined with 1 letter variables that makes for not good code
If I were to build an OO library, I would also play with constructor and add some support for private.

All in all, this is something I would not mind using, but that I would hate to have to maintain.